-
Notifications
You must be signed in to change notification settings - Fork 108
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
Fix arguments comparison logic for overlapping fields #164
Conversation
I'll update the PR to match the implementation in |
00292f8
to
2675fc6
Compare
for _, arg1 := range args1 { | ||
var match *ast.Argument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could probably just be a bool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was trying to follow this logic. I can change it to boolean.
"github.com/vektah/gqlparser/v2/ast" | ||
. "github.com/vektah/gqlparser/v2/validator" | ||
"reflect" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unintended change?
2675fc6
to
2c30ea7
Compare
Hello - Any chance we can get a review on this PR? |
I have just hit this as a blocking issue :sadpanda: I've created an alternative that includes tests @ #170 |
Closing in favor of #170 but thanks for getting this started! |
The current argument comparison seems different from
graphql-js
which is resulting in unexpected behavior like this.Instead of comparing all the arguments to each other, we just need to compare the corresponding ones (with the same
argument.Name
)