search

graphql-elixir / graphql

GraphQL Elixir
Other
859 stars 47 forks source link

[WIP] Convert AST to use Erlang records and Elixir structs #19

Closed joshprice closed 8 years ago

joshprice commented 8 years ago

Mainly for comparison/discussion with @bruce about PR #18

bruce commented 8 years ago

Some initial thoughts:

I'd put the AST-related modules/structs under a common module name (eg, GraphQL.AST) to more clearly break them away from the rest of the package, and probably remove the kind field (since the struct knows what it is).

I don't see a benefit to using records; Elixir structs could be created in build_ast_node via, eg:

%% Expecting `Node` to be a map, as in #18
build_ast_node(Type, Node, Loc) ->
  Node#{'__struct__' => list_to_atom("Elixir.GraphQL.AST." ++ atom_to_list(Type)), loc => Loc}.

(I'm experimenting with these ideas in another implementation, too, that's a more direct port of the reference implementation -- since I need to work through the concepts quickly for a project, and that's the most direct path. My hope is to get that to a point where I can open it that up sometime this weekend and we can do some more cross-polination -- in a concrete vs abstract manner.)

joshprice commented 8 years ago

Agreed @bruce would love to chat about project structure, have been pondering the best way to go with this, but don't have enough of a sense of the Elixir conventions yet. My thinking was to try and follow the graphql-js reference implementation structure.

joshprice commented 8 years ago

The kind field was there only for compatibility with the reference impl.

Also the plan is to convert the maps to structs like you describe, took a while to realise that the Elixir. prefix was required for the __struct__ trick to work.

Look forward to seeing what you come up with!

joshprice commented 8 years ago

Closing this as plain maps are just fine