-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add option to specify config path argument name #334
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #334 +/- ##
==========================================
- Coverage 87.06% 87.03% -0.03%
==========================================
Files 34 34
Lines 4538 4543 +5
==========================================
+ Hits 3951 3954 +3
- Misses 587 589 +2 ☔ View full report in Codecov by Sentry. |
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.
Thanks a lot @joshlk , this looks good! There's just a minor comment here about the extra restriction that you place on the values that config_path_name
can take.
simple_parsing/parsing.py
Outdated
if isinstance(add_config_path_arg, str) and not add_config_path_arg.isidentifier(): | ||
raise ValueError( | ||
"If `add_config_path_arg` is a string it must be a valid Python identifier (no dashes)." | ||
f" Not: {add_config_path_arg}" | ||
) |
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.
Actually, you can use attribute names with dashes when you getattr
and setattr
on a Namespace
object:
>>> from argparse import Namespace
>>> ns = Namespace()
>>> setattr(ns, "some-attr", 42)
>>> getattr(ns, "some-attr")
42
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, if you have the time, would you mind adding a simple test so we can cover this change?
@pytest.mark.parametrize("invalid_value", ["foo.bar.baz?", "-------", "bob bob bob"]) # add other more relevant examples perhaps?
def test_pass_invalid_value_to_add_config_path_arg(invalid_value):
with pytest.raises(ValueError):
_ = ArgumentParser(add_config_path_arg=invalid_value)
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.
Actually, you can use attribute names with dashes when you
getattr
andsetattr
on aNamespace
object:>>> from argparse import Namespace >>> ns = Namespace() >>> setattr(ns, "some-attr", 42) >>> getattr(ns, "some-attr") 42
Thanks good idea. I have it working now. For reference, you have to replace the dashes with underscores like so:
getattr(args_with_config_path, config_path_name.replace('-', '_'))
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, if you have the time, would you mind adding a simple test so we can cover this change?
@pytest.mark.parametrize("invalid_value", ["foo.bar.baz?", "-------", "bob bob bob"]) # add other more relevant examples perhaps? def test_pass_invalid_value_to_add_config_path_arg(invalid_value): with pytest.raises(ValueError): _ = ArgumentParser(add_config_path_arg=invalid_value)
I've added some tests
simple_parsing/parsing.py
Outdated
@@ -1010,10 +1027,12 @@ def parse( | |||
|
|||
If `config_path` is passed, loads the values from that file and uses them as defaults. | |||
""" | |||
if dest == add_config_path_arg: | |||
raise ValueError("`add_config_path_arg` cannot use the same name as `dest`.") |
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.
raise ValueError("`add_config_path_arg` cannot use the same name as `dest`.") | |
raise ValueError("`add_config_path_arg` cannot be the same as `dest`.") |
@lebrice I've addressed the comments. Its ready for a re-review |
rename arg
Add an option so the user can specify the name of the config path argument.
I have extended the
add_config_path_arg
parameter ofArgumentParser
to accept a string. If specified this determines the name of the argument, otherwise, it defaults to the previous value ofconfig_path
.I have also added the
config_path
parameter toArgumentParser
's docstrings, as it was missing previously.