-
Notifications
You must be signed in to change notification settings - Fork 790
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
Properly handle nargs=-1 for Click API #2219
Conversation
@@ -319,7 +333,7 @@ def getattr_wrapper(_self, name): | |||
defaults, | |||
) = extract_all_params(cli_collection) | |||
|
|||
def _method(_self, **kwargs): | |||
def _method(_self, *args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to use the *args
introduced here somewhere?
This is because we do use **kwargs
inside _method_sanity_check
later..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, without it it didn't work for functions that have args. I think the signature changes or something.
@@ -379,6 +393,9 @@ def execute(self) -> List[str]: | |||
else: | |||
components.append("--%s" % k) | |||
components.append(str(i)) | |||
elif v is None: | |||
continue # Skip None values -- they are defaults and converting | |||
# them to string will not be what the user wants |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any quick example for cases where we get None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, what about checking for v is None
when looping over args.items()
this check is only for looping over options.items()
do we need it? just in case..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could but I don't think we would hit it. This case happens basically when a default value of None was passed on the command line. It transformed into a None string.
No description provided.