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

Ensure OperationDefinition case has at least one selection #24

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

AndrewSisley
Copy link
Contributor

Posibly theoretical, but is an easy todo to kill off and its better to have the defensive code than a panic

Posibly theoretical, but is an easy todo to kill off and its better to have the defensive code than a panic
@AndrewSisley AndrewSisley self-assigned this Nov 1, 2021
@AndrewSisley
Copy link
Contributor Author

A whole branch PR is a tad overkill here perhaps, but it is an indepentant todo...

Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you rebase this off of master, the build should pass.

As for the size of the PR thats OK 😂. I wasn't too worried about this possible branch because the GraphQL parser would complain before we got to this point.

However, in the instance where you manually create the *parser.Query object this could be a condition, so its good to have this covered.

Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to mark the review as "Request changes".

Just rebase and we can merge

@AndrewSisley AndrewSisley merged commit d1ff999 into master Nov 2, 2021
@AndrewSisley AndrewSisley deleted the check-for-atleast-one-selection branch November 2, 2021 12:35
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…ast-one-selection

Ensure OperationDefinition case has at least one selection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants