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 python -W with respect to PipDeprecationWarning #3455

Merged
merged 3 commits into from Feb 11, 2016

Conversation

Projects
None yet
2 participants
@bukzor
Contributor

bukzor commented Feb 5, 2016

Before:

$ python2.6 -W ignore -m pip.__main__ list |& head
DEPRECATION: Python 2.6 is no longer supported by the Python core team, please upgrade your Python. A future version of pip will drop support for Python 2.6
adns-python (1.2.1)
argparse (1.1)
Babel (0.9.4)
boto (2.34.0)
bounce-studio (3.7.0.570)
bpython (0.9.5.2)
buildbot (0.8.3-yelp6)
buildbot-slave (0.8.3-yelp6)
catbox (1.7.0)

After: (note warning is correctly ignored)

$ python2.6 -W ignore -m pip.__main__ list |& head
adns-python (1.2.1)
argparse (1.1)
Babel (0.9.4)
boto (2.34.0)
bounce-studio (3.7.0.570)
bpython (0.9.5.2)
buildbot (0.8.3-yelp6)
buildbot-slave (0.8.3-yelp6)
catbox (1.7.0)
Chameleon (2.10)

Implementation:

Default warning filters should be appended to the filter chain, to allow filters added by users via -W (et. al.) to have a chance to run. This has a problem when the Warnings are derived from DeprecationWarning. The docs state that the default behavior for DeprecationWarning is ignore. Appending to the filter chain won't change this. Since this isn't the desired behavior for PipDeprecationWarning, I believe it's incorrect to derive from this warning, albeit the names are similar.

Changing PipDeprecationWarning to derive directly from Warning allows the appended rule to be the default behavior, while also allowing rules prepended by users to have effect.

Testing:

I really can't see where a test for this would fit into the current test suite. Suggestions are very welcome. I should assert that:

  1. The default behavior for PipDeprecation is to print a red error.
  2. The behavior of (PipDeprecation, Pending) is to print a yellow warning.
  3. Using -W (or PYTHONWARNINGS or similar) can change the behavior to ignore, or error.

Review on Reviewable

Show outdated Hide outdated pip/utils/deprecation.py Outdated
class PipDeprecationWarning(Warning):
pass
class RemovedInPip9Warning(PipDeprecationWarning, DeprecationWarning):
class Pending(object):

This comment has been minimized.

@xavfernandez

xavfernandez Feb 7, 2016

Contributor

A docstring with the warning/error distinction would be nice

@xavfernandez

xavfernandez Feb 7, 2016

Contributor

A docstring with the warning/error distinction would be nice

This comment has been minimized.

@bukzor

bukzor Feb 8, 2016

Contributor

The comment you're looking for is on line 62. That much hasn't changed.

@bukzor

bukzor Feb 8, 2016

Contributor

The comment you're looking for is on line 62. That much hasn't changed.

Show outdated Hide outdated pip/index.py Outdated
@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Feb 7, 2016

Contributor

Good catch for the missing append=True :)

Concerning the tests, I think they are a good idea.
Maybe not the yellow/red distinction but at least verify the possibility for users to hide them in the stderr output.

Contributor

xavfernandez commented Feb 7, 2016

Good catch for the missing append=True :)

Concerning the tests, I think they are a good idea.
Maybe not the yellow/red distinction but at least verify the possibility for users to hide them in the stderr output.

@bukzor

This comment has been minimized.

Show comment
Hide comment
@bukzor

bukzor Feb 8, 2016

Contributor

@xavfernandez Thanks. The question was where to put the test and how to implement them. I don't find anything comparable in the current test suite.

Contributor

bukzor commented Feb 8, 2016

@xavfernandez Thanks. The question was where to put the test and how to implement them. I don't find anything comparable in the current test suite.

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Feb 8, 2016

Contributor

Well, I'd go with a tests/functional/test_warning.py with maybe something like:

def test_python26(script):
    result = script.run('python', '-m', 'pip.__main__', 'list', expect_stderr=True)
    assert "Python 2.6 is no longer supported" in result.stderr
    result = script.run('python', '-W', 'ignore', '-m', 'pip.__main__', 'list')
    assert "Python 2.6 is no longer supported" not in result.stderr

and for python2.7+, use --no-binary ?

Contributor

xavfernandez commented Feb 8, 2016

Well, I'd go with a tests/functional/test_warning.py with maybe something like:

def test_python26(script):
    result = script.run('python', '-m', 'pip.__main__', 'list', expect_stderr=True)
    assert "Python 2.6 is no longer supported" in result.stderr
    result = script.run('python', '-W', 'ignore', '-m', 'pip.__main__', 'list')
    assert "Python 2.6 is no longer supported" not in result.stderr

and for python2.7+, use --no-binary ?

@xavfernandez xavfernandez added this to the 8.1 milestone Feb 8, 2016

bukzor referenced this pull request in Yelp/venv-update Feb 8, 2016

@xavfernandez xavfernandez modified the milestones: 8.0.3, 8.1 Feb 10, 2016

@pytest.mark.skipif("sys.version_info < (2,7)")
def test_environ(script, tmpdir):
"""$PYTHONWARNINGS was added in python2.7"""
demo = tmpdir.join('warnings_demo.py')

This comment has been minimized.

@xavfernandez

xavfernandez Feb 10, 2016

Contributor

I'm not sure about this functional test as it is more testing pip internals than a real use case...

@xavfernandez

xavfernandez Feb 10, 2016

Contributor

I'm not sure about this functional test as it is more testing pip internals than a real use case...

This comment has been minimized.

@bukzor

bukzor Feb 10, 2016

Contributor

@xavfernandez Please suggest a course of action. Is this ok to ship?

My rationale is this is only way to assert this behavior without relying on triggering some particular deprecation, which will change with time, and may be extremely hard / impossible to trigger in future.

@bukzor

bukzor Feb 10, 2016

Contributor

@xavfernandez Please suggest a course of action. Is this ok to ship?

My rationale is this is only way to assert this behavior without relying on triggering some particular deprecation, which will change with time, and may be extremely hard / impossible to trigger in future.

This comment has been minimized.

@xavfernandez

xavfernandez Feb 11, 2016

Contributor

Yup, I had something like pip install some_local_package --no-binary=some_local_package in mind but indeed, this will only work until --no-binary is effectively dropped so your solution seems indeed more future resilient.

@xavfernandez

xavfernandez Feb 11, 2016

Contributor

Yup, I had something like pip install some_local_package --no-binary=some_local_package in mind but indeed, this will only work until --no-binary is effectively dropped so your solution seems indeed more future resilient.

xavfernandez added a commit that referenced this pull request Feb 11, 2016

Merge pull request #3455 from bukzor/enable-warning-filters
enable python -W with respect to PipDeprecationWarning

@xavfernandez xavfernandez merged commit 8233a83 into pypa:develop Feb 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Feb 11, 2016

Contributor

Thanks for your work 👍

Contributor

xavfernandez commented Feb 11, 2016

Thanks for your work 👍

@bukzor bukzor deleted the bukzor:enable-warning-filters branch Feb 11, 2016

xavfernandez added a commit to xavfernandez/pip that referenced this pull request Feb 24, 2016

Merge pull request pypa#3455 from bukzor/enable-warning-filters
enable python -W with respect to PipDeprecationWarning

@piotr-dobrogost piotr-dobrogost referenced this pull request Mar 16, 2016

Merged

Append warnings. #817

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment