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

Across the codebase, handle the case a keyword argument is provided but set to None #1965

Merged
merged 1 commit into from
Feb 20, 2022

Conversation

janluke
Copy link
Contributor

@janluke janluke commented Jun 18, 2021

In several points, Click relies exclusively on the following check to verify if an argument arg_name has been provided: arg_name in attrs. Then it assumes that attrs[arg_name] is not None. This PR is about treating None values in attrs as "not provided" too.

There are probably other points in the codebase that do the same thing. If you intend to merge this, I'll go on and search more in depth. I'll also add a test and a changelog entry.

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@janluke janluke changed the title In decorators, handle the case an argument is provided but it's None Across the codebase, handle the case a keyword argument is provided but set to None Jun 25, 2021
@davidism davidism added this to the 8.0.2 milestone Jul 4, 2021
@davidism
Copy link
Member

davidism commented Jul 4, 2021

This seems fine, if you want to go through the code more like you mentioned.

I think what I'd really like is to stop using **kwargs, but it gets pretty verbose to write out all parameters for the subclass constructors and decorators. Would make IDEs and type checkers smarter. That can come later though.

@davidism
Copy link
Member

davidism commented Jul 4, 2021

That one seems to have broken a behavior.

Besides that change, were there any others? Let me know when this is ready for merge.

@janluke
Copy link
Contributor Author

janluke commented Jul 4, 2021

My last commit broke a test. This is due to the fact that default=None is not equivalent to not providing default for binary flags:

  • when you don't provide default, default is set to False
  • when default=None, default is set to None.

I think this may be accidental behavior but it's not senseless: settings default=None you can force the prompt to require an explicit value to be provided (it'll be [y/n] rather than [y/N]).

Nonetheless, I noticed that if you provide default=None to a flag without a secondary option, the prompt message is broken: it won't include "[y/n]" at the end.

@davidism I think it's better to handle this issue in another PR eventually. Thus, I'd revert my last commit here. What do you think?

@davidism
Copy link
Member

davidism commented Jul 4, 2021

Sounds good to me, I'll merge this one after you remove that commit.

@davidism davidism modified the milestones: 8.0.2, 8.1.0 Jul 4, 2021
@janluke
Copy link
Contributor Author

janluke commented Jul 4, 2021

I reverted the commit, I didn't remove it. This is ready to be merged.

Besides that change, were there any others? Let me know when this is ready for merge.

I searched for "in attrs" and "in kwargs". There are a few other places where Click relies on these but they seem legit.

@davidism davidism merged commit c9c406b into pallets:main Feb 20, 2022
@janluke janluke deleted the fix/handle-none branch February 20, 2022 17:33
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Param decorators assume attrs[key] is not None just because key in attrs
2 participants