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

Fix a minor bug in the GraphQL formatter. #30

Merged
merged 1 commit into from Aug 15, 2017
Merged

Conversation

obi1kenobi
Copy link
Contributor

Here's the error I was getting.

File /.../graphql_compiler/query_formatting/graphql_formatting.py", in leave_Directive
    arg = args.pop(arg_names.index(defined_arg))
IndexError: pop index out of range

I wasn't able to come up with a deterministic test case that reproduces the problem, but I know the underlying cause. The problem happens because the args and arg_names lists were expected to be parallel, but popping could happen in any order. The error above is caused when the lists cease to be parallel, e.g. as follows:

********* before first pop, popping at index 0

['op_name: "="', 'value: ["$parameter"]']
['op_name', 'value']

********* before second pop, popping at index 1

['value: ["$parameter"]']
['op_name', 'value']

********* IndexError since there was no index

```
File /.../graphql_compiler/query_formatting/graphql_formatting.py", in leave_Directive
    arg = args.pop(arg_names.index(defined_arg))
IndexError: pop index out of range
```

This happened because the `args` and `arg_names` lists were expected to be parallel, but popping could happen in any order. The bug above is caused when the lists cease to be parallel, e.g. as follows:

```
********* before first pop, popping at index 0

['op_name: "="', 'value: ["$parameter"]']
['op_name', 'value']

********* before second pop, popping at index 1

['value: ["$parameter"]']
['op_name', 'value']

********* IndexError since there was no index
```
@obi1kenobi
Copy link
Contributor Author

@benlongo if you have a moment, since I'm modifying your code, I'd appreciate your feedback as well. Also, thank you for the thorough unit tests you wrote -- they were very helpful in making sure I didn't add more bugs by accident :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 93.72% when pulling eb6258d on fix_formatting_bug into a36ac09 on master.

@benlongo
Copy link
Contributor

What a curious little bug - the changes look good, I probably should've gone with the set approach to begin with 👍

@obi1kenobi
Copy link
Contributor Author

Hindsight is 20/20 after all :) Thanks for reviewing so quickly!

@obi1kenobi obi1kenobi merged commit 9262d22 into master Aug 15, 2017
@obi1kenobi obi1kenobi deleted the fix_formatting_bug branch August 15, 2017 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants