Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
  • 12:49

    StevenACoffman on master

    Check for non-enum values on va… (compare)

  • 12:49
    StevenACoffman closed #227
  • 12:26
    coveralls commented #227
  • 12:25
    AmeyaRao98 synchronize #227
  • 12:22
    AmeyaRao98 edited #227
  • 12:22
    coveralls commented #227
  • 12:21
    AmeyaRao98 opened #227
  • Jun 13 17:05

    StevenACoffman on v2.4.5

    (compare)

  • Jun 06 23:46

    StevenACoffman on master

    Validate that the operation's r… (compare)

  • Jun 06 23:46
    StevenACoffman closed #225
  • Jun 06 23:46
    StevenACoffman closed #221
  • Jun 06 21:04
    StevenACoffman commented #225
  • Jun 06 20:53

    StevenACoffman on master

    Allow doubly-defined builtin di… (compare)

  • Jun 06 20:53
    StevenACoffman closed #226
  • Jun 06 20:53
    StevenACoffman closed #223
  • Jun 06 20:52
    StevenACoffman commented #226
  • Jun 06 20:19
    benjaminjkraft commented #226
  • Jun 06 20:19
    coveralls commented #226
  • Jun 06 20:17
    benjaminjkraft opened #226
  • Jun 06 19:54
    coveralls commented #225
Masahiro Wakame
@vvakame
I'd tried below patter.
// use i here instead of `i, sel` otherwise you take a pointer to a copy.
for i := range field.SelectionSet {
    w.walkSelection(def, &field.SelectionSet[i])
}

func (w *Walker) walkSelection(parentDef *ast.Definition, it *ast.Selection) {
    // ...
}
Adam Scarr
@vektah
as you said, Selection is an interface
so all they are already pointers
or is the problem that the thing in the interface isnt a pointer?
Masahiro Wakame
@vvakame
type Selection interface {
    isSelection()
}

func (Field) isSelection()          {}
func (FragmentSpread) isSelection() {}
func (InlineFragment) isSelection() {}
problems is here. Selection implemented by Field, FragmentSpread, InlineFragment. not Field, not FragmentSpread, *InlineFragment.
wow, italic..
Adam Scarr
@vektah
feel free to make them func(*Field)?
it will need a few changes to the parser, but that shouldn't be too bad?
Masahiro Wakame
@vvakame
maybe yes.
Masahiro Wakame
@vvakame
my objective is implement OverlappingFieldsCanBeMerged.
Adam Scarr
@vektah
yeah, whats the problem you're running into?
Masahiro Wakame
@vvakame
wait a moment, I'll write problmen's description again.
Masahiro Wakame
@vvakame

I need to visit all SelectionSet and it has Definitions (e.g. Field with FieldDefinition)

  • All sub nodes have Definition too
    • InlineFragment's fields...
    • FragmentSpread's fields...
    • deeply nested foobar's fields...

How can the above be achieved?
I tried below code.

    observers := w.Observers
    w.Observers = &Events{}
    w.walk()

    w.Observers = observers
    w.walk()

However, there were a lot of code call by value and the value was still nil.
(and It's a bit inefficient.)

Do I have to use function arguments as pointers?
It is necessary to change to the following code.

    for idx := range w.Document.Operations {
        child := &w.Document.Operations[idx]
        w.validatedFragmentSpreads = make(map[string]bool)
        w.walkOperation(child)
    }

and... Selection seems difficult unless we change the definition.

Writing as usual would do as follows.
This is a confusing bug that values are ghostly eaten.
    for _, child := range w.Document.Operations {
        w.validatedFragmentSpreads = make(map[string]bool)
        w.walkOperation(&child)
    }
Masahiro Wakame
@vvakame
I will return home from now so check here again later!
Adam Scarr
@vektah

right, so because

type QueryDocument struct {
    Operations []OperationDefinition
}

operations is all allocated contiguously in memory (its very efficient to loop over due to cache coherency, but adding elements can be costly)

Operations: [OperationDefinition(Operation: "query", Name: "", ...)][OperationDefinition(Operation: "query", Name: "", ...)][OperationDefinition(Operation: "query", Name: "", ...)]

the problem is a for loop copies it when traversing:

for i, def := range document.Operation {
    // &def is a pointer to a copy
   // &document.Operation[i] is a pointer to the element of the array
}

wheras if

type QueryDocument struct {
    Operations []*OperationDefinition // note pointer
}

you end up with an array of pointers, referencing objects randomly allocated all over the place

op 3: [OperationDefinition(Operation: "query", Name: "", ...)]
[ptr to 1][ptr to 2][ptr to 3]
op 1: [OperationDefinition(Operation: "query", Name: "", ...)]
op 2: [OperationDefinition(Operation: "query", Name: "", ...)]

when you loop over the pointers, go still copies them, but its a copy of a pointer so it doesnt matter so much, the cost you pay is now your reading things from random locations in memory as you loop, which is much slower if you do it a lot (for operation it probably doesnt matter, you expect a few items max

feel free to make []*things wherever you need, we can always wind them back to []things if they are expected to be lager
pretty sure contiguous arrays are also less pressure on the gc, not having to clean up lots of small references. perf without benchmarks is dumb though, so do what makes sense
Masahiro Wakame
@vvakame
okay, I'll try it later.
Masahiro Wakame
@vvakame
I started going forward!
Adam Scarr
@vektah
and I broke all your code?
:D
vektah/gqlparser#39 makes all the collections []*Thing
Masahiro Wakame
@vvakame
yeah I found it and merge it in 1 min :smiley_cat:
Masahiro Wakame
@vvakame
@vektah I have a question.
https://github.com/vektah/gqlparser/blob/master/spec/validation/deviations.yml#L13-L26
Why these tests are marked skip: true ?
Adam Scarr
@vektah
inherited from the upstream library
we should try and get them implemented
Masahiro Wakame
@vvakame
make sense :thinking_face:
Masahiro Wakame
@vvakame
From tomorrow I will return to another job that is stacked. :wink:
Please let me know if you have anything you would like me to help with.
I can not do much as I go to SanFransisco next week.
Adam Scarr
@vektah
oh wow you finished it!
amazing :heart_eyes:
Adam Scarr
@vektah
@vvakame I've sent you an collaborator invite for gqlparser
I'm tracking stuff for "1.0" on https://github.com/vektah/gqlgen/projects/1, if you see something you want to work on feel free
just drag it into "in progress" so someone else doesn't start working on it
I think Mat will start to break down that directive card some more once he gets his head around how it will all hoook in
Masahiro Wakame
@vvakame
wow, thx. I accepted invitation.
Adam Scarr
@vektah
thanks for all the hard work :D
Masahiro Wakame
@vvakame
If you did not make your code at first, I would not be eager to GraphQL. thanks for your hard work too :sparkles:
Tamal Saha
@tamalsaha
Hello, just trying to learn about GraphQL in Go.
Have you looked into using https://github.com/graphql-go/graphql/tree/master/language parser instead of writing your own?
Adam Scarr
@vektah
Yeah, I looked at a bunch
the parser and lexer are actually really easy
the validation is hard
Adam Scarr
@vektah
That library also doesnt use any of the schema IDL, so while there is code there its not being used for anything

also line count comparisons are interesting,

   110 ./benchutil/list_schema.go
   144 ./benchutil/wide_schema.go
  1311 ./definition.go
   148 ./directives.go
    74 ./examples/context/main.go
   141 ./examples/custom-scalar-type/main.go
    41 ./examples/hello-world/main.go
   114 ./examples/http/main.go
   138 ./examples/httpdynamic/main.go
    75 ./examples/modify-context/main.go
    24 ./examples/star-wars/main.go
   225 ./examples/todo/main.go
   872 ./executor.go
    78 ./gqlerrors/error.go
    62 ./gqlerrors/formatted.go
    39 ./gqlerrors/located.go
    30 ./gqlerrors/sortutil.go
    69 ./gqlerrors/syntax.go
    63 ./graphql.go
   763 ./introspection.go
    29 ./language/ast/arguments.go
   235 ./language/ast/definitions.go
    33 ./language/ast/directives.go
    31 ./language/ast/document.go
    22 ./language/ast/location.go
    28 ./language/ast/name.go
    45 ./language/ast/node.go
   134 ./language/ast/selections.go
   103 ./language/ast/types.go
   529 ./language/ast/type_definitions.go
   302 ./language/ast/values.go
    59 ./language/kinds/kinds.go
   682 ./language/lexer/lexer.go
    35 ./language/location/location.go
  1607 ./language/parser/parser.go
   822 ./language/printer/printer.go
    20 ./language/source/source.go
    11 ./language/typeInfo/type_info.go
   736 ./language/visitor/visitor.go
    51 ./located.go
  1892 ./rules.go
   706 ./rules_overlapping_fields_can_be_merged.go
   433 ./scalars.go
   538 ./schema.go
    99 ./testutil/introspection_query.go
   622 ./testutil/rules_test_harness.go
   468 ./testutil/testutil.go
    16 ./types.go
   276 ./type_info.go
   192 ./util.go
   287 ./validator.go
   421 ./values.go
 15985 total

vs

   138 ./ast/collections.go
    92 ./ast/definition.go
    38 ./ast/directive.go
    65 ./ast/document.go
   159 ./ast/dumper.go
    38 ./ast/fragment.go
    29 ./ast/operation.go
    35 ./ast/selection.go
    14 ./ast/source.go
    68 ./ast/type.go
   115 ./ast/value.go
    88 ./gqlerror/error.go
    35 ./gqlparser.go
    58 ./lexer/blockstring.go
   510 ./lexer/lexer.go
   148 ./lexer/token.go
   112 ./parser/parser.go
   334 ./parser/query.go
   503 ./parser/schema.go
   138 ./parser/testrunner/runner.go
    55 ./validator/error.go
    30 ./validator/inliner/inliner.go
    39 ./validator/messaging.go
     5 ./validator/prelude.go
    86 ./validator/rules/fields_on_correct_type.go
    39 ./validator/rules/fragments_on_composite_types.go
    57 ./validator/rules/known_argument_names.go
    31 ./validator/rules/known_directives.go
    19 ./validator/rules/known_fragment_names.go
    61 ./validator/rules/known_type_names.go
    19 ./validator/rules/lone_anonymous_operation.go
    93 ./validator/rules/no_fragment_cycles.go
    28 ./validator/rules/no_undefined_variables.go
    30 ./validator/rules/no_unused_fragments.go
    30 ./validator/rules/no_unused_variables.go
   553 ./validator/rules/overlapping_fields_can_be_merged.go
    68 ./validator/rules/possible_fragment_spreads.go
    63 ./validator/rules/provided_required_arguments.go
    36 ./validator/rules/scalar_leafs.go
    30 ./validator/rules/single_field_subscriptions.go
    33 ./validator/rules/unique_argument_names.go
    24 ./validator/rules/unique_directives_per_location.go
    22 ./validator/rules/unique_fragment_names.go
    27 ./validator/rules/unique_input_field_names.go
    22 ./validator/rules/unique_operation_names.go
    23 ./validator/rules/unique_variable_names.go
   130 ./validator/rules/values_of_correct_type.go
    28 ./validator/rules/variables_are_input_types.go
    36 ./validator/rules/variables_in_allowed_position.go
   199 ./validator/schema.go
    69 ./validator/suggestionList.go
    44 ./validator/validator.go
   286 ./validator/walk.go
  5032 total
Tamal Saha
@tamalsaha
Thanks @vektah . With every release of GraphQL does the language change?
Adam Scarr
@vektah
yeah, gqlparser is importing the testsuite from graphql/graphql-js to help keep up to date