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

Use log level info when ignoring packages due to environment markers #4877

Merged
merged 1 commit into from
Mar 1, 2018
Merged

Use log level info when ignoring packages due to environment markers #4877

merged 1 commit into from
Mar 1, 2018

Conversation

edmorley
Copy link
Contributor

The use of environment markers implies that the user expects the packages to not be installed in some cases (eg depending on version of Python), so the log output shouldn't be classed as a warning, particularly since this results in it being sent to stderr rather than stdout.

Fixes #4876.

@pradyunsg pradyunsg added type: enhancement Improvements to functionality C: logging Information Logging labels Nov 21, 2017
@edmorley
Copy link
Contributor Author

edmorley commented Dec 6, 2017

Is there someone who has a spare moment to review this? :-)

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

Sounds and looks fine to me.

@GabrielC101
Copy link
Contributor

I'm a little concerned about this.

If the user is specifying an extra, via pip install package_name[extra_name], it's because the user expects extra_name to be available.

An unavailable extra is an error.

Can this be handled in a way that treats failed extra evaluations as errors, but other evaluations as info events?

@GabrielC101
Copy link
Contributor

Never mind. If the user requests a non-existent extra, the logging takes place at https://github.com/pypa/pip/blob/master/src/pip/_internal/resolve.py#L295.

The use of environment markers implies that the user expects the
packages to not be installed in some cases (eg depending on version
of Python), so the log output shouldn't be classed as a warning,
particularly since this results in it being sent to `stderr` rather
than `stdout`.

Fixes #4876.
@edmorley
Copy link
Contributor Author

I've rebased this on master.
It has one review approval - is another required, or can this be merged? :-)

@pradyunsg pradyunsg requested a review from a team January 28, 2018 16:49
@pradyunsg
Copy link
Member

I'd like one more.

@pfmoore
Copy link
Member

pfmoore commented Jan 28, 2018

I'm fine with the implementation of the change, but I'm not completely convinced by the argument for why it's needed - It feels correct to me that if we ignore a requirement, that's a warning, not an informational message. But the distinction between warn and info is so subtle in practice, that I don't care a whole lot.

So @pradyunsg if you feel OK with switching the level of this message, I'm happy to be your second approval that it's implemented correctly. If you're after approval that the change of level is OK, then I'd say "don't know, but I doubt it's a big deal either way".

@edmorley
Copy link
Contributor Author

edmorley commented Jan 28, 2018

Hi! Thank you for taking a look :-)

Can you think of some example use-cases where skipping a requirement really is something of concern that needs a warning on stderr? #4876 has a use-case for the reverse - and I'd suspect that's more common.

@pradyunsg
Copy link
Member

(sorry for the terse statement before.)

@pfmoore I agree with everything you said.

I'm slightly more accepting of this change mostly because I'd prefer spewing less text into stderr for that reason I'm cool with doing a change in level here.

@pradyunsg pradyunsg mentioned this pull request Mar 1, 2018
@pradyunsg pradyunsg added this to the 10.0 milestone Mar 1, 2018
@pfmoore pfmoore merged commit 1cb99c1 into pypa:master Mar 1, 2018
@edmorley edmorley deleted the markers-ignore-loglevel branch March 2, 2018 16:01
@edmorley
Copy link
Contributor Author

edmorley commented Mar 2, 2018

Many thanks :-)

@lock
Copy link

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: logging Information Logging type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Messages about ignoring packages due to environment markers should not be on stderr
4 participants