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 new --with-blanklines option to pip freeze command #12485

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

G000D1ESS
Copy link

@G000D1ESS G000D1ESS commented Jan 25, 2024

This pull request (PR) introduces a new option to the pip freeze command. New --with-blanklines option allows you to preserve blank lines in final output when using the --requirement option, which can be convenient and useful in certain scenarios.

For example, when updating dependencies, it will be based on requirements.txt:

# databases
alembic==1.13.1
asyncpg==0.29.0
SQLAlchemy==2.0.25
psycopg2==2.9.9

# network
aiohttp==3.9.1
requests==2.31.0

# parsing
bs4==0.0.1
Scrapy==2.11.0

# math
numpy==1.26.3

The requirements.txt format will be preserved without violations, including the retention of empty lines in output.

# databases
alembic==1.13.1
asyncpg==0.29.0
SQLAlchemy==2.0.25
psycopg2==2.9.9

# network
aiohttp==3.9.1
requests==2.31.0

# parsing
bs4==0.0.1
Scrapy==2.11.0

# math
numpy==1.26.3
## The following requirements were added by pip freeze:
...other packages...

Instead of default behavior:

# databases
alembic==1.13.1
asyncpg==0.29.0
SQLAlchemy==2.0.25
psycopg2==2.9.9

# network
aiohttp==3.9.1
requests==2.31.0
# parsing
bs4==0.0.1
Scrapy==2.11.0
# math
numpy==1.26.3
## The following requirements were added by pip freeze:
...other packages...

@sbidoul
Copy link
Member

sbidoul commented Jan 29, 2024

Thanks for submitting this.

I'm personally -1 to add a new option just for this. If this is deemed important, it could be enabled by default.

I'm not a user of that particular pip freeze feature myself, though, and I'm not sure if introducing this would be considered a backward incompatible change.

@sbidoul sbidoul added the C: freeze 'pip freeze' related label Jan 29, 2024
@G000D1ESS
Copy link
Author

Thanks for the answer.

I agree that adding a new flag maybe is unnecessary. There are two aspects why I decided to make this flag optional:

  1. Backward compatibility. The old logic has been around for 8 years
  2. When using multiple requirements.txt, it may not be convenient to use it in the mode with saving empty lines by default.

But if you think this should work by default, then I can work in that direction.

@sbidoul
Copy link
Member

sbidoul commented Jan 30, 2024

Can you elaborate item 2?

@G000D1ESS
Copy link
Author

About the second item. For example we have two requirements.txt - first.txt and second.txt:

The first.txt is

# databases
alembic==1.13.1
SQLAlchemy==2.0.25
psycopg2==2.9.9

# network
aiohttp==3.9.1

# parsing
bs4==0.0.1

And second.txt is

# databases
alembic==1.13.1
asyncpg==0.29.0
SQLAlchemy==2.0.25
psycopg2==2.9.9

# network
requests==2.31.0

# parsing
Scrapy==2.11.0

If we pass it to pip freeze command without new flag (pip freeze -r first.txt -r second.txt). The output will be like this:

# databases
alembic==1.13.1
SQLAlchemy==2.0.25
psycopg2==2.9.9

# network
aiohttp==3.9.1
# parsing
bs4==0.0.1
asyncpg==0.29.0
requests==2.31.0
Scrapy==2.11.0
## The following requirements were added by pip freeze:
...other packages...

And with new --with-blanklines option (pip freeze -r first.txt -r second.txt --with-blanklines) the output will be:

# databases
alembic==1.13.1
SQLAlchemy==2.0.25
psycopg2==2.9.9

# network
aiohttp==3.9.1

# parsing
bs4==0.0.1
asyncpg==0.29.0

requests==2.31.0

Scrapy==2.11.0
## The following requirements were added by pip freeze:
...other packages...

Preserved blanklines in this case make the output look strange (for me).

@G000D1ESS
Copy link
Author

So, what do you think about it? Should I work in that way or move to make this logic work by default?

@G000D1ESS
Copy link
Author

So, @sbidoul what do you think about this?

@sbidoul
Copy link
Member

sbidoul commented Apr 1, 2024

I'm not sure. This, as an option, sounds very "ad-hoc". I'd prefer a solution that works well out of the box.

BTW, about your example 2, the blank line is not stranger than having requests appearing under the # parsing comment as it does today already?

On the other hand, changing the output by default might be considered a breaking change.

It might be interesting to hear from other users of the pip freeze --requirement feature, as I am not one myself.

@pfmoore
Copy link
Member

pfmoore commented Apr 1, 2024

I agree with @sbidoul - I don't use the pip freeze --requirement but none of the examples given look any worse or better to me. I'm not at all clear what the point is of the option, or what its intended use case is. Ultimately, you either have a machine generated requirements file or a manually maintained one, and this seems like some sort of attempt to create a hybrid "do what I mean" format.

I would be interested in seeing the motivation for the original addition of this option. The ability to use a single --requirements option appears to have been present from the origins of pip, but the intent (from what the docs said) seemed to be simply to ensure the environment was included in the resulting file, not to maintain any particular structure to the file. The ability to use multiple requirement files was added in #3599, as far as I can see because prior to that, specifying multiple files caused the first one(s) to be ignored - at some point the error that the original code flagged when it saw multiple -r options seems to have been lost. But in adding this we seem to have more or less accidentally added a capability of merging multiple -r files while adding any extra requirements from the environment.

The whole thing seems to be largely unintended behaviour.

My preference would be:

  1. Don't merge this change.
  2. To be explicit, and to set expectations, document that the layout of the generated file is intentionally undefined.
  3. Ideally, reinstate the behaviour of only allowing a single -r, and giving an error if multiple occurrences are given.

In practice, though, I don't think (3) is worth the effort, as we clearly have people using the feature. But I don't think we should encourage this usage, or spend maintainer time on it.

Again, though, I'll note that I have no use for the pip freeze -r feature, so my views should be understood from that perspective.

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

Successfully merging this pull request may close these issues.

None yet

3 participants