-
Notifications
You must be signed in to change notification settings - Fork 14
feat: add defaults argument #129
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
Conversation
| hasdefault = param.default is not param.empty | ||
| default = param.default if hasdefault else SUPPRESS | ||
|
|
||
| if not hasdefault and defaults is not None and name in defaults: |
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.
question: This makes the function argument take precedence. Perhaps it should be the other way around?
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.
It would seem that if you pass an explicit default, that one should be preferred?
| Command line arguments to parse (default: ``sys.argv[1:]``). | ||
| :param defaults: | ||
| Mapping for argument defaults passed to | ||
| `~argparse.ArgumentParser.set_defaults`. Key must be the command name, |
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.
chore: fixme, this isn't true anymore
|
I don't know how exactly does your want your yaml-loading works (e.g. how do you specify the import defopt
from functools import partial
def foo(*, bar: int) -> None:
print(bar)
def foo2(*, bar: int) -> None:
print(bar)
if __name__ == '__main__':
defopt.run({"foo": partial(foo, bar=5), "sub": [partial(foo2, bar=6)]})Thoughts? |
|
proper support for functools.partial Would be great. |
|
From a quick test the following patch seems enough? Still needs tests and docs of course. diff --git i/src/defopt.py w/src/defopt.py
index 8855c05..df9e386 100644
--- i/src/defopt.py
+++ w/src/defopt.py
@@ -330,7 +330,8 @@ def _recurse_functions(funcs, subparsers):
# If this iterable is not a mapping, then convert it to one using the
# function name itself as the key, but replacing _ with -.
try:
- funcs = {func.__name__.replace('_', '-'): func for func in funcs}
+ funcs = {_unwrap_partial(func).__name__.replace('_', '-'): func
+ for func in funcs}
except AttributeError as exc:
# Do not allow a mapping inside of a list
raise ValueError(
@@ -468,12 +469,16 @@ def signature(func: Union[Callable, str]):
inspect_sig = _preprocess_inspect_signature(
func, inspect.signature(func))
doc_sig = _preprocess_doc_signature(
- func, signature(inspect.getdoc(func)))
+ func, signature(inspect.getdoc(_unwrap_partial(func))))
return _merge_signatures(inspect_sig, doc_sig)
+def _unwrap_partial(func):
+ return func.func if isinstance(func, functools.partial) else func
+
+
def _preprocess_inspect_signature(func, sig):
- hints = typing.get_type_hints(func)
+ hints = typing.get_type_hints(_unwrap_partial(func))
parameters = []
for name, param in sig.parameters.items():
if param.name.startswith('_'): |
|
Closing in favor of #130 |
The following (incomplete) pull request adds a new argument to the
runcommand to specify a mapping of defaults for function arguments. It may be useful to have the defaults be set at runtime, for example if we want to read them from a YAML or other configuration file. The below is a toy example of how I'd use the argument.If you like the path this PR is going, the next steps would be: