jazzband / pip-tools

A set of tools to keep your pinned Python dependencies fresh.
https://pip-tools.rtfd.io
BSD 3-Clause "New" or "Revised" License
7.6k stars 607 forks source link

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

Closed mjsir911 closed 4 months ago

mjsir911 commented 4 months ago

Fixes #2065

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

Contributor checklist
Maintainer checklist
chrysle commented 4 months ago

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 commented 4 months ago

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:

https://github.com/jazzband/pip-tools/blob/11971512a541f914869603d0d2abcffd8c659fc4/piptools/utils.py#L341-L343

https://github.com/jazzband/pip-tools/blob/11971512a541f914869603d0d2abcffd8c659fc4/piptools/utils.py#L392-L394

& Yeah reuse-hashes is the default: https://github.com/jazzband/pip-tools/blob/11971512a541f914869603d0d2abcffd8c659fc4/piptools/scripts/options.py#L229-L237

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"

chrysle commented 4 months ago

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 commented 4 months ago

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!

mjsir911 commented 4 months ago

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 commented 4 months ago

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.