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

Argument default value should be undefined #1300

Closed

Conversation

wakemaster39
Copy link

resolves #1195

This stops null from being populated in the introspection query as mentioned in #1195. This also solves the problem of None being passed into the kwargs of a mutate call if no value was passed in by the end user.

@wakemaster39
Copy link
Author

I am unsure which of the two options you want me to do to fix the unit tests.

1, set default values for the failing tests to be None.
2, remove the no longer passed in values from the assertion statements.

@DonQueso89
Copy link

I am unsure which of the two options you want me to do to fix the unit tests.

1, set default values for the failing tests to be None.
2, remove the no longer passed in values from the assertion statements.

thumbs up for this PR!

I would say remove the no longer passed in values from the assertion right? We're testing whether the value is absent if its not passed in

Copy link
Contributor

@zbyte64 zbyte64 left a comment

Choose a reason for hiding this comment

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

See the commit to fix the test.

assert (
arg_name not in arguments
), f'More than one Argument have same name "{arg_name}".'
assert arg_name not in arguments, f'More than one Argument have same name "{arg_name}".'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is causing the test to fail. It applies black formatting to the code an fails if the file changes. This is where it cites the changed file: https://travis-ci.org/github/graphql-python/graphene/jobs/753187127#L243

@wakemaster39
Copy link
Author

wakemaster39 commented Mar 1, 2021

Ok I finally got around to cleaning up the changes that moving to Undefined makes to the unit tests. This should be good to go now. Fixed the fact I forgot to run pre-commit along the way.

Copy link
Contributor

@zbyte64 zbyte64 left a comment

Choose a reason for hiding this comment

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

👍

@ramyak-mehra
Copy link

What's the updated on this one?

@lovetoburnswhen
Copy link

+1 this seems like a critical feature

@SteinRobert
Copy link

Does this PR need any more work? I'd be happy to help, otherwise I would really appreciate this to make it into the next release 😃

@frankchen211
Copy link

any update?

@SteinRobert
Copy link

@Cito @jkimbo you guys probably have a lot of work on your hands, thank you so much for caring about the project!
Just wanted to check in whether this change is something you consider accepting or if it needs any more work. I'd surely help making any more changes if needed.

@erikwrede
Copy link
Member

erikwrede commented Aug 13, 2022

Hey everyone,
sorry for the long wait; this fix is already merged:
19ebf08
It should be included in 3.1.0

Closing this.

@erikwrede erikwrede closed this Aug 13, 2022
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.

Default value for Argument's should be Undefined
10 participants