search

graphql-elixir / graphql

GraphQL Elixir
Other
858 stars 45 forks source link

Extract Name as a type in parser #31

Closed plasticine closed 8 years ago

plasticine commented 8 years ago

This PR adds support in the parser for extracting the Name GraphQL type.

The reference implementation (as far as I can tell) uses a Name type instead of the literal string value for name fields in the graph parser currently used in the implementation here.

For example, currently GraphQL.Lang.Parser.parse("{ a, b { x }, c }") would result in Fields that look like this: 

%{kind: :Field, loc: %{start: 0}, name: "c"}

Where according to the reference implementation it should look more like this;

%{kind: :Field, loc: %{start: 0}, name: %{kind: :Name, loc: %{start: 0}, value: "c"}}

Relevant sections of the reference implementation;

TODOs

joshprice commented 8 years ago

Sorry I haven't responded on this earlier, was a bit distracted by the holidays!

This looks pretty good on first inspection. Is it ready to merge?

I can't remember the exact reason for not doing this initially, but I do recall it didn't seem to add much value. I don't think I've seen a reason yet to have an extra layer of indirection on this, but consistency is definitely as good a reason as any to get this in.

plasticine commented 8 years ago

No worries — busy time of year! ;)

Yep, it's good to go.

On Sun, Jan 3, 2016 at 1:27 PM, Josh Price notifications@github.com wrote:

Sorry I haven't responded on this earlier, was a bit distracted by the holidays! This looks pretty good on first inspection. Is it ready to merge?

I can't remember the exact reason for not doing this initially, but I do recall it didn't seem to add much value. I don't think I've seen a reason yet to have an extra layer of indirection on this, but consistency is definitely as good a reason as any to get this in.

Reply to this email directly or view it on GitHub: https://github.com/joshprice/graphql-elixir/pull/31#issuecomment-168450476