Skip to content

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.

3 participants