purenix icon indicating copy to clipboard operation
purenix copied to clipboard

Published 20 hours ago • verified publisher icon purenix-org

Escaping fix + test suite

Open jonascarpay opened this issue 3 years ago • 5 comments

Fixes #35 Fixes #5

This PR contains a rudimentary test suite, as well as a fix for the escaping issue.

  • [x] More tests for string edge cases
  • [x] ~Don't actually just show in Haskell~ the more I think about it the more I think this is fine
  • [x] Run on CI
  • [x] Better type for run

jonascarpay avatar Jan 28 '22 20:01 jonascarpay

I'm on mobile, which doesn't show when or how this got marked as ready for review, and I can't change it back. But just to be sure, it's not ready yet, I'll request review when it is.

jonascarpay avatar Feb 02 '22 02:02 jonascarpay

@jonascarpay No problem, I switched it back to draft for you.

cdepillabout avatar Feb 02 '22 04:02 cdepillabout

After some offline discussion, we decided that the test suite needs a better design before being worth merging.

The issue with this test suite is that, while it looks like a suite of unit tests, it acts more like an integration test. Put differently, the most interesting part about it isn't that it passes all the tests, but that it runs at all. Either we acknowledge that and have a design that better communicates that fact, or we need to find a way of better isolating the guarantees that we want our test suite to provide.

jonascarpay avatar Feb 12 '22 08:02 jonascarpay

hi @jonascarpay cool, that you are adressing those issues! Today I got aware of a thing, that may be helpful: On main the commit 72cc3ffdea361322c169fdbe1c9572117dcda075 introduces a bug that causes escape sequences like \x001b[32m to be printed as ESC[32m when logged/traced to the commandline. It should actually turn the following text's color to be green instead. I tried it with the f29c0149c4d473d9326208ce293239ac354b206b from your PR branch and it's the same behavior. I cannot investigate details right now, If it's not related I can open up a separated issue.

m-bock avatar Feb 22 '22 06:02 m-bock

@thought2 Oh wow, that's a very good catch! Please open a separate issue, and I'll have a think how to best go about fixing it. I guess that with our strings passing through Nix, PureScript, Haskell (and arguably Bash) just using show, and that handling all our escaping concerns was too good to be true :smiling_face_with_tear: .

jonascarpay avatar Feb 22 '22 06:02 jonascarpay