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.

Sign up for GitHub

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

Jump to bottom

Add validation to prevent arguments with both required and default_value #3011

Merged
merged 4 commits into from
Jul 21, 2020
Merged

Add validation to prevent arguments with both required and default_value #3011

merged 4 commits into from
Jul 21, 2020

Conversation

Diego-Thomaz
Copy link
Contributor

Close #3001

lairtonlelis
lairtonlelis suggested changes Jul 1, 2020
View reviewed changes
Copy link
Contributor

@lairtonlelis lairtonlelis left a comment

Choose a reason for hiding this comment

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

Just a small change 💅 . The rest LGTM.

spec/graphql/schema/argument_spec.rb Outdated Show resolved Hide resolved
@Diego-Thomaz @lairtonlelis
Update spec/graphql/schema/argument_spec.rb
588a9c1 
Co-authored-by: Lairton Lélis <lairtonlelis@gmail.com>
rmosolgo
rmosolgo reviewed Jul 11, 2020
View reviewed changes
Copy link
Owner

@rmosolgo rmosolgo left a comment

Choose a reason for hiding this comment

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

Thanks for taking a crack at this! I left a couple of suggestions.

lib/graphql/schema/argument.rb Outdated Show resolved Hide resolved
lib/graphql/schema/argument.rb Show resolved Hide resolved
@Diego-Thomaz
Copy link
Contributor Author

@rmosolgo take a look and let me know if it is ok now!

rmosolgo
rmosolgo approved these changes Jul 21, 2020
View reviewed changes
Copy link
Owner

@rmosolgo rmosolgo left a comment

Choose a reason for hiding this comment

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

Awesome, thanks again for taking care of this!

@rmosolgo rmosolgo added this to the 1.11.2 milestone Jul 21, 2020
@rmosolgo rmosolgo merged commit 7121474 into rmosolgo:master Jul 21, 2020
@rmosolgo rmosolgo mentioned this pull request Aug 7, 2020
Merged
@rmosolgo
Copy link
Owner

rmosolgo commented Aug 7, 2020

Sorry @Diego-Thomaz , I made a mistake on this one -- apparently there are cases where default_value: and required: true make sense (graphql/graphql-spec#418). Maybe I named required: wrongly 😖

@Diego-Thomaz
Copy link
Contributor Author

Sorry @Diego-Thomaz , I made a mistake on this one -- apparently there are cases where default_value: and required: true make sense (graphql/graphql-spec#418). Maybe I named required: wrongly 😖

No problem @rmosolgo I was glad to help anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lairtonlelis lairtonlelis lairtonlelis approved these changes

@rmosolgo rmosolgo rmosolgo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.11.2
Development

Successfully merging this pull request may close these issues.

Arguments should prevent _both_ default_value and required: true
3 participants
@Diego-Thomaz @rmosolgo @lairtonlelis