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

Add --split-annotate option #1297

Closed
wants to merge 1 commit into from
Closed

Conversation

rsalmaso
Copy link

@rsalmaso rsalmaso commented Jan 5, 2021

Add --split-annotate option to pip-compile to allow annotations to be formatted one per line

This is a partial revert of #1237 to restore the default behavior up to 5.4.0

(this is currently a WIP: I need to update tests)

Contributor checklist
  • Provided the tests for the changes.
  • Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@jdufresne
Copy link
Member

I'm -0 on this change.

I think having many multiple output formats and many CLI arguments to control them is layering complexity. The PR size may look small, but complexity comes in the form of code, tests, and help requests. New features must now consider multiple output formatting paths. So, I prefer to be a bit opinionated in the formatting so long as the output is correct, informative, and useful.

If for some reason we must include a new argument to control the output format, I think we should move towards the new expanded annotations as the default. So I'd like to see some warning in the documentation and in the console that instructs users to use --split-annotate to migrate to the new format or --no-split-annotate for the old formatting. But again, my preference is to not add this argument.

@adamchainz
Copy link
Contributor

I'm also against this for the same reasons that @jdufresne notes. As he noted in this comment the rendered requirements.txt file should not be considered a stable format, beyond being a valid file for pip. This is just like Gemfile.lock, package-lock.json, yarn.lock, ...

@feelepxyz
Copy link

requirements.txt file should not be considered a stable format, beyond being a valid file for pip. This is just like Gemfile.lock, package-lock.json, yarn.lock

Chiming in from a third-party tooling perspective. Dependabot creates automated pull requests with dependency updates and whenever these formats change the format that users generate when running the tool locally might be different from what is generated by Dependabot. Suddenly receiving a different format is surprising and at worst a breaking change when you haven't explicitly asked for it. The cli version you use might also have existing workflow dependencies that make that upgrade hard.

We have historically had a grace period for upgrading major package mangers versions in Dependabot where we run both and switch at runtime based on the lockfile format.

@scholarsmate
Copy link

For now, I'm just removing the comments altogether. Not ideal, but we need to be pragmatic.

grep -v '#' requirements.txt >requirements.new && mv requirements.new requirements.txt

@AndydeCleyre
Copy link
Contributor

AndydeCleyre commented Jan 8, 2021

In addition to breaking existing methods of working with the output, and furthering this project from one of its strengths as a line-oriented tool, I am against the new multi-line annotation style because I can see drastically less information on my monitor at once now.

EDIT: Please, think of those of us with small screens. magic-wormhole via pip-tools 5.4.0 on the left, 5.5.0 on the right:

image

@akx
Copy link

akx commented Jan 12, 2021

Maybe this could be made more future-proof as

--annotation-style=none  # new alias for --no-annotate
--annotation-style=split  # the new style
--annotation-style=long  # the old default

or similar?

@fdemmer
Copy link

fdemmer commented Jan 12, 2021

Please seriously consider this or a similar PR to (optionally) revert back to single-line per package formatting. The new format makes simple commandline operations like using grep to quickly find usage and dependencies of a single package impossible, eg:

$ grep cairo requirements/base.txt 
cairocffi==1.2.0
    #   cairosvg
cairosvg==2.5.1
    #   cairocffi
    #   cairosvg
    #   cairosvg
    #   cairosvg
    #   cairosvg

vs old:

$ grep cairo requirements/base.txt 
cairocffi==1.2.0          # via cairosvg, weasyprint
cairosvg==2.5.1           # via weasyprint
cffi==1.14.4              # via cairocffi, cryptography, weasyprint
cssselect2==0.4.1         # via cairosvg, weasyprint
defusedxml==0.6.0         # via cairosvg, python3-openid, social-auth-core
pillow==7.2.0             # via -r base.in, bokeh, cairosvg, easy-thumbnails, matplotlib, qrcode, wagtail, weasyprint
tinycss2==1.1.0           # via cairosvg, cssselect2, weasyprint

@adamchainz
Copy link
Contributor

adamchainz commented Jan 12, 2021

Or you could use grep '^cairo' to match the start of the line. (See Python's regex syntax)

@wodCZ
Copy link

wodCZ commented Jan 12, 2021

Or you could use grep '^cairo' to match the start of the line. (See Python's regex syntax)

I believe the point was not in finding the dependency itself, but finding which packages use the dependency.

@adamchainz
Copy link
Contributor

Then it's grep '# cairo'

@wodCZ
Copy link

wodCZ commented Jan 12, 2021

Which would produce following with the new format 👀

    #   cairosvg
    #   cairocffi
    #   cairosvg
    #   cairosvg
    #   cairosvg
    #   cairosvg

@adamchainz
Copy link
Contributor

Right, this can be done with ripgrep like so (swapped to 'django' so I can use a requirements fine at hand):

$ rg --multiline '^[^ #].*$\n(^( )+#.*\n)*^( )+#   django$' requirements.txt
133:pytz==2020.5
134:    # via
135:    #   apscheduler
136:    #   django
144:sqlparse==0.4.1
145:    # via
146:    #   django

It's probably possible with grep as well, but it seems the multiline flag is non standard. One can also use python's re module.

Anyway the point is not that we should retain an old format just because one regex stopped working.

@fdemmer
Copy link

fdemmer commented Jan 12, 2021

i am not here to argue. your project, your decision. from my point of view you just removed a key feature (easy way to find packages and packages depending on one another using simple tooling) to gain some nicer formatting, that could have been achieved by configuring line-breaks in an editor. that is unfortunate. thank you for providing the ridiculous regex to still somewhat achieve my use case.

@AndydeCleyre
Copy link
Contributor

For me the new impractical use of space is such a regression that I will (poorly) maintain a fork of this project if this isn't fixed in the main repo. I searched but did not find an existing issue tracking this -- is there one?

@AndydeCleyre
Copy link
Contributor

OK, I've made #1306 to track the request.

@az-pz
Copy link

az-pz commented Jan 25, 2021

@fdemmer , pip-deptree --reverse --package <package-name> would be useful to find the packages dependent on . pip-deptree comes with pip-tools. I know it's not as convenient as grep but it is the best way (imho) if you are using pip-tools.

@AndydeCleyre
Copy link
Contributor

I just want to invite folks here to check out alternative PR #1477, and provide feedback so we can nail this down. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants