Skip to content
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

Fix pip-compile to properly exclude non-relevant options from output header #2066

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

mjsir911
Copy link
Contributor

@mjsir911 mjsir911 commented Mar 5, 2024

Fixes #2065

Click context only had positive --reuse-hashes to loop over, inverse is automatically deduced.

Contributor checklist
  • Included tests for the changes.
  • PR title is short, clear, and ready to be included in the user-facing changelog.
Maintainer checklist
  • Verified one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@chrysle chrysle added bug Something is not working logging Related to log or console output labels Mar 5, 2024
@chrysle
Copy link
Contributor

chrysle commented Mar 5, 2024

Thanks! As it should be worthwhile to include --reuse-hashes in the output header, could you try the following logic to test for the negative forms of boolean flags in COMPILE_EXCLUDE_OPTIONS?

diff --git a/piptools/utils.py b/piptools/utils.py
index 3b2061f..fc03846 100644
--- a/piptools/utils.py
+++ b/piptools/utils.py
@@ -371,9 +371,13 @@ def get_compile_command(click_ctx: click.Context) -> str:
         # Get the latest option name (usually it'll be a long name)
         option_long_name = option.opts[-1]
 
+        negative_option = None
+        if option.is_flag:
+            negative_option = f"--no-{option_long_name.lstrip('--')}"
+
         # Exclude one-off options (--upgrade/--upgrade-package/--rebuild/...)
         # or options that don't change compile behaviour (--verbose/--dry-run/...)
-        if option_long_name in COMPILE_EXCLUDE_OPTIONS:
+        if option_long_name or negative_option in COMPILE_EXCLUDE_OPTIONS:
             continue

Moreover, since the pull request title will be included in the user-facing changelog, and I categorise this as a bugfix rather than a feature, as the title suggests, would you be fine with changing it to something like "Fix superfluous inclusion of --no-reuse-hashes flag in pip-compile output header"?

@mjsir911
Copy link
Contributor Author

mjsir911 commented Mar 5, 2024

As it should be worthwhile to include --reuse-hashes in the output header

As it currently stands, pip-compile header doesn't include options passed with default values:

pip-tools/piptools/utils.py

Lines 341 to 343 in 1197151

The command will be normalized by:
- expanding options short to long
- removing values that are already default

pip-tools/piptools/utils.py

Lines 392 to 394 in 1197151

# Skip options with a default value
if option.default == value:
continue

& Yeah reuse-hashes is the default:

reuse_hashes = click.option(
"--reuse-hashes/--no-reuse-hashes",
is_flag=True,
default=True,
help=(
"Improve the speed of --generate-hashes by reusing the hashes from an "
"existing output file."
),
)

Even if the above wasn't the case, your suggested changes wouldn't allow through a --reuse-hashes because option_long_name is still being filtered against. You'de have to do something like:

if option.is_flag and value == False:
    option_long_name = negative_option

I can still apply your suggested changes, looks to be more clear and allows for embedding the inverse flags within the COMPILE_EXCLUDE_OPTION.

I do have to push back on the suggested commit summary change though, especially with your changes it does become solely a "fix get_compile_command to properly filter out specified inverse command arguments"

@mjsir911 mjsir911 force-pushed the main branch 3 times, most recently from 7a3e028 to afea6e6 Compare March 5, 2024 17:58
@chrysle
Copy link
Contributor

chrysle commented Mar 6, 2024

Even if the above wasn't the case, your suggested changes wouldn't allow through a --reuse-hashes because option_long_name is still being filtered against.

Ah, you're correct, I didn't realise that.

especially with your changes it does become solely a "fix get_compile_command to properly filter out specified inverse command arguments"

How about "Fix pip-compile to properly exclude non-relevant options from output header"? The internal API should not be relevant to (typical) end users.

@mjsir911
Copy link
Contributor Author

mjsir911 commented Mar 6, 2024

How about "Fix pip-compile to properly exclude non-relevant options from output header"? The internal API should not be relevant to (typical) end users.

Works for me, thanks for helping me along with this!

@chrysle chrysle changed the title Add non-inverse flag to COMPILE_EXCLUDE_OPTIONS Fix pip-compile to properly exclude non-relevant options from output header Mar 6, 2024
@mjsir911
Copy link
Contributor Author

mjsir911 commented Mar 6, 2024

OH I did misunderstand @chrysle I was focusing on the commit message. does that get added to the changelog too or is it purely the PR title?

@chrysle
Copy link
Contributor

chrysle commented Mar 9, 2024

or is it purely the PR title?

Yes, since we're using the release-drafter GH Action, but we're planning to move away from it.

@chrysle chrysle added this to the 7.4.2 milestone Mar 9, 2024
Fixes: jazzband#2065
See: jazzband#1197

This is specifically for the --no-reuse-hashes found in
COMPILE_EXCLUDE_OPTIONS but makes way for future inverse flags in the
list too.
@chrysle chrysle enabled auto-merge March 11, 2024 09:04
@chrysle chrysle added this pull request to the merge queue Mar 11, 2024
Merged via the queue into jazzband:main with commit 4f44a02 Mar 11, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working logging Related to log or console output
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flag in COMPILE_EXCLUDE_OPTIONS showing up in header
2 participants