Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escaping fix + test suite #44

Closed
wants to merge 4 commits into from
Closed

Escaping fix + test suite #44

wants to merge 4 commits into from

Conversation

jonascarpay
Copy link
Member

@jonascarpay jonascarpay commented Jan 28, 2022

Fixes #35
Fixes #5

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

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

@jonascarpay jonascarpay marked this pull request as ready for review January 31, 2022 12:55
@jonascarpay
Copy link
Member Author

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.

@cdepillabout cdepillabout marked this pull request as draft February 2, 2022 04:01
@cdepillabout
Copy link
Member

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

@jonascarpay jonascarpay force-pushed the escape-fix branch 2 times, most recently from ccbcd05 to ccaa6b0 Compare February 5, 2022 16:52
@jonascarpay jonascarpay marked this pull request as ready for review February 5, 2022 17:02
Comment on lines +3 to +4
data TestCase
data Result
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it really matters for this PR, but I thought you might be interested that it seems like PureScript people like to use empty data types for things like Void, but use foreign import data for things that are coming from FFI:

https://discourse.purescript.org/t/what-does-using-data-without-any-assignments-mean-i-e-data-something/1651/5?u=cdepillabout

@jonascarpay
Copy link
Member Author

jonascarpay commented Feb 12, 2022

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 jonascarpay marked this pull request as draft February 12, 2022 08:58
@jonascarpay jonascarpay mentioned this pull request Feb 12, 2022
@m-bock
Copy link

m-bock commented Feb 22, 2022

hi @jonascarpay
cool, that you are adressing those issues! Today I got aware of a thing, that may be helpful:
On main the commit 72cc3ff 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 f29c014 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.

@jonascarpay
Copy link
Member Author

@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 🥲 .

@jonascarpay jonascarpay closed this May 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test suite characters in quotes aren't escaped correctly
3 participants