Refactor how Nox defines and process options #187
Conversation
|
This is a huge change and I'm really crossing my fingers that I didn't break anything in process, but the tests seem minimally affected. |
|
/cc @dhermes @lukesneeringer @stsewd please don't feel obligated to, but if y'all have some time to look at this monster change I'd really appreciate it. If not, that's totally fine! I'll merge it in 2 days. |
| ): | ||
| self.name = name | ||
| self.flags = flags | ||
| self.help = help |
There was a problem hiding this comment.
I'll echo what I said earlier about shadowing __builtins__.id (now you are shadowing help)
There was a problem hiding this comment.
I know, but I want/need to match argparse's signature for things that are directly passed through.
There was a problem hiding this comment.
kwargs.pop('help') allows you to match the signature without having a shadowed local
| specified on the command-line.""" | ||
| noxfile_value = getattr(noxfile_args, enable_name) | ||
| command_value = getattr(command_args, enable_name) | ||
| disable_value = getattr(command_args, disable_name) |
There was a problem hiding this comment.
I'm not a big fan of getattr(). Not sure what's going on in this function (i.e. what types are the args? the return isn't guaranteed to be a boolean?)
There was a problem hiding this comment.
Do you have a suggestion for alternative to getattr here?
I added some more details to the docstrings to hopefully explain what this function is doing. Let me know if that helps any.
There was a problem hiding this comment.
This is essentially runtime code generation. You could instead do build-time code generation.
There was a problem hiding this comment.
Could you expand on that?
There was a problem hiding this comment.
I was suggesting that you write a script which takes some text file (or DSL) of your choosing and spits out code for you. This way you can explicitly define your flags while getting code that's easy to reason about (getattr(foo, bar) ends up being a lot of indirection as compared to foo.known_attr).
There was a problem hiding this comment.
I see. I think that DSLs and code generation tend to be a lot harder to understand and reason about than some light metaprogramming, but I understand the concern here. I don't think we've crossed the line where inventing such a thing is worth the trade off, but gosh if we ever have to do something like this for another project then yeah.
There was a problem hiding this comment.
Yeah a DSL would be overkill.
| """ | ||
| disable_name = "no_{}".format(name) | ||
|
|
||
| kwargs["action"] = "store_true" |
There was a problem hiding this comment.
Not worried if action was set by the caller?
There was a problem hiding this comment.
No, it shouldn't be. We (the Nox developers) are the only ones using this, and flag pairs are/must be boolean.
|
Thank you so much for taking a look @dhermes. I'll try to address all these comments tomorrow or Saturday. I apologize for how big this is, but hopefully how we express our options is "clearer". |
nox/_option_set.py
Outdated
| command_args, name, option.merge_func(command_args, noxfile_args) | ||
| ) | ||
| elif option.noxfile: | ||
| value = getattr(command_args, name) or getattr(noxfile_args, name) |
There was a problem hiding this comment.
getattrwill raise an exception if a default value isn't given. You need to pass None explicitly. Anyway, I'm not sure if it's guarantied that command_args always have the name attr
There was a problem hiding this comment.
They should always have the same set of properties since they're derived from the same source, but added the None default anyway.
| ): | ||
| self.name = name | ||
| self.flags = flags | ||
| self.help = help |
There was a problem hiding this comment.
I know, but I want/need to match argparse's signature for things that are directly passed through.
| specified on the command-line.""" | ||
| noxfile_value = getattr(noxfile_args, enable_name) | ||
| command_value = getattr(command_args, enable_name) | ||
| disable_value = getattr(command_args, disable_name) |
There was a problem hiding this comment.
Do you have a suggestion for alternative to getattr here?
I added some more details to the docstrings to hopefully explain what this function is doing. Let me know if that helps any.
| """ | ||
| disable_name = "no_{}".format(name) | ||
|
|
||
| kwargs["action"] = "store_true" |
There was a problem hiding this comment.
No, it shouldn't be. We (the Nox developers) are the only ones using this, and flag pairs are/must be boolean.
| @@ -235,5 +280,7 @@ def merge_namespaces(self, command_args, noxfile_args): | |||
| command_args, name, option.merge_func(command_args, noxfile_args) | |||
| ) | |||
| elif option.noxfile: | |||
| value = getattr(command_args, name) or getattr(noxfile_args, name) | |||
| value = getattr(command_args, name, None) or getattr( | |||
There was a problem hiding this comment.
Ha, big difference here 😀
tests/test_main.py
Outdated
| sys.argv = [sys.executable] | ||
| with mock.patch("nox.workflow.execute") as execute: | ||
| execute.return_value = 0 | ||
| with mock.patch("sys.stderr.isatty") as istty: |
There was a problem hiding this comment.
ISTR you prefer naming the mock object the same as the thing being mocked, in which case istty -> isatty
theacodes
force-pushed
the
refactor-options
branch
from
26693af
to
9986efb
Compare
Closes #146, although there will need to be a follow up to use this information in the documentation.