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

Fix double escaped back slashes in string arguments #962

Merged
merged 2 commits into from
Aug 24, 2020
Merged

Fix double escaped back slashes in string arguments #962

merged 2 commits into from
Aug 24, 2020

Conversation

mattbaker
Copy link
Contributor

@mattbaker mattbaker commented Aug 1, 2020

@binaryseed suggested I just submit the branch I used to test for #961 as a PR with failing tests as an example. Happy to update the parser to remove the escape processing and add it to this PR if that's the right direction.

@mattbaker mattbaker changed the title Add tests showing string argument escaping behavior Test parsing of multiple escaped back slashes in string arguments Aug 1, 2020
@benwilson512
Copy link
Contributor

@bruce any chance you can weigh in on this one?

@bruce
Copy link
Contributor

bruce commented Aug 9, 2020

@mattbaker Thanks for submitting this. I've made a commit that removes the escape handling from the parser code, relying instead on the lexer to handle it. Tests are passing locally. Please take a look.

@mattbaker
Copy link
Contributor Author

Makes sense to me @bruce 👍 The only thing on my mind is that this may actually be a breaking change, in case that impacts how the change is released or communicated.

I suspect it's rare for people to be escaping so heavily in a string, we just happen to have a couple specific cases in New Relic's API where folks are (which is for us to figure out).

@binaryseed binaryseed changed the title Test parsing of multiple escaped back slashes in string arguments Fix double escaped back slashes in string arguments Aug 24, 2020
@binaryseed binaryseed merged commit 7796984 into absinthe-graphql:master Aug 24, 2020
@mattbaker mattbaker deleted the escape-escape branch September 12, 2020 23:32
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.

None yet

4 participants