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

Enable single-line annotations with pip-compile --annotation-style=line #1477

Merged
merged 3 commits into from Sep 20, 2021

Conversation

AndydeCleyre
Copy link
Contributor

@AndydeCleyre AndydeCleyre commented Aug 29, 2021

pip-compile gains --annotation-style=line which uses the recently discontinued single-line annotation format.

The current default behavior is unchanged, but can be explicitly passed as --annotation-style=split.

This aims to fix #1306. Please provide feedback!

Contributor checklist
  • Provided the tests for the changes.
  • Assure PR title is short, clear, and good to be included in the user-oriented changelog
Maintainer checklist
  • Assure 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).

@AndydeCleyre AndydeCleyre marked this pull request as draft August 29, 2021 04:54
@AndydeCleyre AndydeCleyre changed the title [WIP] Enable single line annotation comments with pip-compile --annotation-style=line Enable single line annotation comments with pip-compile --annotation-style=line Aug 29, 2021
@AndydeCleyre AndydeCleyre added annotations Related to packages annotations feature labels Aug 29, 2021
@AndydeCleyre AndydeCleyre changed the title Enable single line annotation comments with pip-compile --annotation-style=line Enable single-line annotations with pip-compile --annotation-style=line Aug 29, 2021
@reinout
Copy link

reinout commented Aug 31, 2021

Small detail: the term "split". Would "verbose" or "multiline" be friendlier?

The verbose, multiline output is the default, so it seems a bit weird to use the term "split" as it is only "split" as opposed to the non-default "line" version.

Would a --single-line option without parameter be clearer? Then you only need to explain that it restores the old single-line-behaviour. You don't need to figure out a name for the default behaviour.

@AndydeCleyre
Copy link
Contributor Author

AndydeCleyre commented Aug 31, 2021

Thanks for the feedback!

Small detail: the term "split". Would "verbose" or "multiline" be friendlier?

Multiline seems fine, but I don't prefer verbose, as we could have a more verbose single line annotation, theoretically. And it's not really more verbose anyway (no extra words).

Whatever maintainers and community want to call it.

Would a --single-line option without parameter be clearer?

Well not that exactly, because that doesn't indicate that this only affects the annotation (hashes still get their own lines). But the question of a single flag vs a parameter style argument is a good one.


I don't personally care about these details, and am going to mark this as ready for review to hopefully gather answers to these and other questions from other maintainers.

@AndydeCleyre AndydeCleyre marked this pull request as ready for review August 31, 2021 14:48
@jdufresne

This comment has been minimized.

@AndydeCleyre

This comment has been minimized.

@jdufresne

This comment has been minimized.

@AndydeCleyre

This comment has been minimized.

@AndydeCleyre AndydeCleyre mentioned this pull request Aug 31, 2021
3 tasks
@AndydeCleyre

This comment has been minimized.

@rsalmaso
Copy link

rsalmaso commented Sep 2, 2021

Nice! I was thinking about the same approach as #1297 was really a not good feature.

Copy link
Member

@ulope ulope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments.
In general looks like it's doing what it says on the tin :)

piptools/writer.py Outdated Show resolved Hide resolved
tests/test_cli_compile.py Show resolved Hide resolved
@AndydeCleyre

This comment has been minimized.

@jdufresne

This comment has been minimized.

@AndydeCleyre

This comment has been minimized.

@jdufresne

This comment has been minimized.

@AndydeCleyre

This comment has been minimized.

Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍🏻 Thanks for the work!


I see the continuation backslash on the last generated --hash option is back again (gone in 5.5.0, see #1237). I presume there'll be some noise in users' VCS. Are we sure this is okay?

@atugushev atugushev added this to the 6.3.0 milestone Sep 18, 2021
@AndydeCleyre
Copy link
Contributor Author

Thanks @atugushev!

  • I don't prefer to have that final trailing slash, but the reviewers before you do. 🤷‍♂️
  • Do you think the style names can be improved?

@jdufresne
Copy link
Member

I see the continuation backslash on the last generated --hash option is back again (gone in 5.5.0, see #1237). I presume there'll be some noise in users' VCS. Are we sure this is okay?

Thanks for linking this issue. I forgot that we intentionally removed this when I made a comment above 🤦. Yeah, lets not include the final trailing slash. (Sorry for the back and forth on this.)

Copy link
Member

@jdufresne jdufresne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest lgtm.

@AndydeCleyre
Copy link
Contributor Author

Alright I've removed the continuation between the last hash and the annotation again, with apologies to @merwok and @ulope.

@AndydeCleyre

This comment has been minimized.

@atugushev atugushev closed this Sep 18, 2021
@atugushev atugushev reopened this Sep 18, 2021
@atugushev

This comment has been minimized.

@AndydeCleyre

This comment has been minimized.

@AndydeCleyre

This comment has been minimized.

@AndydeCleyre

This comment has been minimized.

@atugushev

This comment has been minimized.

@atugushev

This comment has been minimized.

@AndydeCleyre

This comment has been minimized.

@AndydeCleyre

This comment has been minimized.

@AndydeCleyre
Copy link
Contributor Author

All set, unless @atugushev or @jdufresne object to the latest commit.

piptools/writer.py Outdated Show resolved Hide resolved
AndydeCleyre and others added 3 commits September 20, 2021 15:04
- Replace the map-based dispatching with more readable blocks
- Catch (unlikely) invalid style value now that it won't raise a KeyError,
  although it should be caught earlier via Click args
- Add a little more comment context to the use of 24 as column size

Co-authored-by: Albert Tugushev <albert@tugushev.ru>
@atugushev
Copy link
Member

Thanks, @AndydeCleyre! 🎉

@atugushev
Copy link
Member

Released in pip-tools v6.3.0 🚀

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

Successfully merging this pull request may close these issues.

Request: Compact annotations
8 participants