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

Separate multiple inherited interfaces with & #1304

Merged
merged 4 commits into from
Feb 28, 2018

Conversation

cjoudrey
Copy link
Contributor

@cjoudrey cjoudrey commented Feb 21, 2018

This pull request modifies the parser and language printer to support separating multiple inherited interfaces with & as per graphql/graphql-js#1169 and graphql/graphql-spec#90.

This replaces:

type Foo implements Bar, Baz { field: Type }

With:

type Foo implements Bar & Baz { field: Type }

With no changes to the common case of implementing a single interface.

In order to preserve backwards compatibility, the parser temporarily supports the old syntax. The language printer only prints the new syntax though.

Note: We need to revert 27ac21c and generate the parser before merging this as the version of ragel on brew is not as recent as the version used here.

@rmosolgo
Copy link
Owner

Wow, I didn't realize this change was in the final draft, interesting. It definitely makes the tokenization easier, I bet atom-language-graphql could finally be fixed.

For compatibility, how about we continue to accept , as well as &? That way, anyone who's building the schema from SDL won't be disrupted. (I know there are a few). But we can always dump the latest (&), so I think that's good as is. Does that sound fair?

@rmosolgo
Copy link
Owner

ragel

Oh my, I should build a Ragel API 😆 I'm pretty sure it's a violation of Ragel's license. (Or... maybe I'd just have to release it GPL or something ...)

@cjoudrey
Copy link
Contributor Author

For compatibility, how about we continue to accept , as well as &?

Sounds good 👍, I'll make that change tonight.

@cjoudrey
Copy link
Contributor Author

bb4cf6f adds backwards compatibility.

Repository owner deleted a comment Feb 28, 2018
@rmosolgo
Copy link
Owner

Ok, I rebuilt the lexer, I'll merge when it's green

@rmosolgo rmosolgo added this to the 1.7.14 milestone Feb 28, 2018
@rmosolgo rmosolgo merged commit ee4c065 into rmosolgo:master Feb 28, 2018
@rmosolgo
Copy link
Owner

Thanks for taking care of this!

@cjoudrey cjoudrey deleted the amp-delimited-implements branch March 16, 2018 03:44
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