-
-
Notifications
You must be signed in to change notification settings - Fork 619
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 typings for the scripts.compile module #1322
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1322 +/- ##
=======================================
Coverage 99.65% 99.65%
=======================================
Files 33 33
Lines 2882 2915 +33
Branches 309 308 -1
=======================================
+ Hits 2872 2905 +33
Misses 5 5
Partials 5 5
Continue to review full report at Codecov.
|
@@ -315,7 +315,7 @@ def get_compile_command(click_ctx: click.Context) -> str: | |||
else: | |||
if isinstance(val, str) and is_url(val): | |||
val = redact_auth_from_url(val) | |||
if option.name == "pip_args": | |||
if option.name == "pip_args_str": |
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.
this doesn't look related to typing
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.
Related to #1322 (comment)
build_isolation: bool, | ||
emit_find_links: bool, | ||
cache_dir: str, | ||
pip_args_str: Optional[str], |
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.
Why did you rename it?
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.
Note, the CLI option is untouched , only variable name’s changed. See option decorator.
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.
There's a lot of changes and renaming variables on top of everything else is hard to follow. It's usually better to make separate patches that are easier to digest for reviewers.
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.
The intention was to make diff less noisy and make PR easier for reviewers.
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.
Would you like me to revert the variable name back? I'm okay with both variants.
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.
Looks like you're changing the CLI API which is unrelated and is a breaking change.
Self-requested review after requested changes doesn't reset red flag
Gentle ping @webknjaz, @jdufresne 👋🏼 All suggestions seem resolved. Is there anything I can do to complete the review? |
Refs: #972