Skip to content

GraphQL::Compatibility #366

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

Merged
merged 9 commits into from
Nov 1, 2016
Merged

GraphQL::Compatibility #366

merged 9 commits into from
Nov 1, 2016

Conversation

rmosolgo
Copy link
Owner

Some parts of GraphQL are (theoretically?) interchangeable. This adds some tests for external users who want to replace some functionality: GraphQL::Compatibility::{Execution,QueryParser,SchemaParser}Specification.

Personally I want this for libgraphqlparser now, it will show us clearly how the Ruby parser differs from libgraphqlparser, here's the PR for that: rmosolgo/graphql-libgraphqlparser-ruby#20

@staugaard thanks for getting this started with ParserTests, I've moved them around a bit but I think I covered all the bases! Some of the behaviors don't match libgraphqlparser so I put them in the gem-specific test file.

@rmosolgo rmosolgo modified the milestone: 1.1.0 Nov 1, 2016
@rmosolgo
Copy link
Owner Author

rmosolgo commented Nov 1, 2016

This also introduces a breaking change ("breaking fix"?) to how we parse string literals. Previously, we wrongly took the sequence '\' 'u' \d \d \d \d and converted it to unicode, now we don't. That is, only "\u" is treated as the unicode escape character, not "\\u". (I noticed this issue when testing against libgraphqlparser, then re-read the spec. Oops!)

@rmosolgo rmosolgo merged commit e320a88 into master Nov 1, 2016
@rmosolgo rmosolgo deleted the compatibility branch November 1, 2016 12:58
@staugaard
Copy link
Contributor

Nice thanks..

On Tue, Nov 1, 2016, 06:00 Robert Mosolgo notifications@github.com wrote:

Merged #366 #366.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#366 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAQzOVKmXqwQFe4z_yulMOG-3K45l0Tks5q5zdpgaJpZM4KkRci
.

@rmosolgo
Copy link
Owner Author

rmosolgo commented Nov 2, 2016

K, that unicode thing was wrong I think, I will put it back:

https://github.com/graphql/graphql-js/blob/master/src/language/__tests__/lexer-test.js#L166-L173

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.

2 participants