refactor: use click in dependency_resolver.py#1071
refactor: use click in dependency_resolver.py#1071rickeylev merged 1 commit intobazel-contrib:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
cj81499
force-pushed
the
use-click
branch
2 times, most recently
from
c40fb0e to
b30e47f
Compare
|
@hrfuller Apologies for the delay. This is ready for review! |
|
@hrfuller are you available to take a look at this? If not, is there someone else who might be able to? |
Yes, I can take a look. @rickeylev is a more active maintainer than me these days so I'd like him to take a look as well. |
hrfuller
left a comment
hrfuller
left a comment
There was a problem hiding this comment.
Overall looks like an improvement to me. Thanks!
| os.environ["LC_ALL"] = "C.UTF-8" | ||
| os.environ["LANG"] = "C.UTF-8" | ||
|
|
||
| argv = [] |
There was a problem hiding this comment.
It looks like we lose sys.argv[0] compared to before. I'm not sure if the piptools cli expects argv to have argv[0] be the name of the original script but maybe we can add sys.argv[0] here in case.
There was a problem hiding this comment.
Just to be clear, are you suggesting we change this to the following?
| argv = [] | |
| argv = [sys.argv[0]] |
There was a problem hiding this comment.
From what I can tell, omitting argv[0] when calling cli() is correct.
Under the hood, piptools is using click, and the implementation is obscured by a ton of decorators. Digging into it...
compile.cliis an instance of the clickCommandclass.Command.__call__is an alias forCommand.mainCommand.maindocs say, for the first positionl arg (namedargs): "the arguments that should be used for parsing. If not provided sys.argv[1:] is used"
So it doesn't expect args to contain argv[0]. The docs go on further to say the optional prog_name arg is initialized to sys.argv[0] if not specified.
As a side note, there is a standalone_mode arg, which controls whether the command tries to exit or not.
| if Path(requirements_in_relative).exists() | ||
| else resolved_requirements_in | ||
| ) | ||
| print(sys.argv) |
There was a problem hiding this comment.
Unclear why this was here. Probably for debugging?
There was a problem hiding this comment.
That was my assumption too. Didn't seem like something that ought to stick around (adding clutter to stdout!).
Happy to add it back if someone has a good reason for it to stay there.
There was a problem hiding this comment.
@rickeylev is a more active maintainer than me these days so I'd like him to take a look as well.
@hrfuller I take this to mean you'd like to hold off on merging until they review?
| os.environ["LC_ALL"] = "C.UTF-8" | ||
| os.environ["LANG"] = "C.UTF-8" | ||
|
|
||
| argv = [] |
There was a problem hiding this comment.
Just to be clear, are you suggesting we change this to the following?
| argv = [] | |
| argv = [sys.argv[0]] |
| if Path(requirements_in_relative).exists() | ||
| else resolved_requirements_in | ||
| ) | ||
| print(sys.argv) |
There was a problem hiding this comment.
That was my assumption too. Didn't seem like something that ought to stick around (adding clutter to stdout!).
Happy to add it back if someone has a good reason for it to stay there.
|
@hrfuller @rickeylev bump! I would really like to get this (and the related #1067) merged. |
|
@hrfuller @rickeylev I'd still like to get this (and #1067) merged. |
|
@f0rmiga I can't seem to get the attention of @hrfuller or @rickeylev and I'm not sure what else to try (other than contacting another codeowner). Perhaps you can help? |
cj81499
force-pushed
the
use-click
branch
2 times, most recently
from
653b91f to
3b413d7
Compare
avoid mutating `sys.argv` Co-authored-by: Logan Pulley <[email protected]>
rickeylev
changed the title
click
| os.environ["LC_ALL"] = "C.UTF-8" | ||
| os.environ["LANG"] = "C.UTF-8" | ||
|
|
||
| argv = [] |
There was a problem hiding this comment.
From what I can tell, omitting argv[0] when calling cli() is correct.
Under the hood, piptools is using click, and the implementation is obscured by a ton of decorators. Digging into it...
compile.cliis an instance of the clickCommandclass.Command.__call__is an alias forCommand.mainCommand.maindocs say, for the first positionl arg (namedargs): "the arguments that should be used for parsing. If not provided sys.argv[1:] is used"
So it doesn't expect args to contain argv[0]. The docs go on further to say the optional prog_name arg is initialized to sys.argv[0] if not specified.
As a side note, there is a standalone_mode arg, which controls whether the command tries to exit or not.
|
Hi @cj81499, Thanks for your PR. Sorry it took so long to be reviewed. I had let this languish because I thought it was introducing a new dependency on click, but we already have that dependency, so this isn't as bad as I thought. |
|
@rickeylev thank you for getting around to this! ❤️ fwiw, I did say the following in the PR description ;)
Excited that #1067 will be unblocked once this makes its way through the queue! |
4 participants
Using click makes it easier to parse arguments. Many args are now named arguments
(options), and the need for using positional args with stub
"None"values isn'tnecessary anymore.
There is already a dependency on click via piptools, so this doesn't introduce a new
dependency.
Relates to #1067