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

bpo-31975 (PEP 565): Show DeprecationWarning in __main__ #4458

Merged

Conversation

ncoghlan
Copy link
Contributor

@ncoghlan ncoghlan commented Nov 19, 2017

  • primary change is to add a new default filter entry for
    default::DeprecationWarning:__main__
  • secondary change is an internal one to cope with plain
    strings in the warning module's internal filter list
    (this avoids the need to create a compiled regex object
    early on during interpreter startup)

https://bugs.python.org/issue31975

@@ -1186,7 +1211,7 @@ def __del__(self):
rc, out, err = assert_python_ok("-c", code)
# note: "__main__" filename is not correct, it should be the name
# of the script
self.assertEqual(err, b'__main__:7: UserWarning: test')
self.assertEqual(err.decode(), '__main__:7: UserWarning: test')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may look unrelated, but comparing these as strings means a proper diff gets printed when the test fails (and it was this test that picked up the problem with the non-regex entry in the internal filter list).

@ncoghlan
Copy link
Contributor Author

ncoghlan commented Nov 19, 2017

@brettcannon Something I noticed while making this, is that we don't currently have a test that ensures the C level filter initialisation is consistent with the Python level default filter initialisation at https://github.com/python/cpython/blob/3.6/Lib/warnings.py#L506

I'm thinking this may be a good opportunity to address that, by adding a variant of the test added by the PR that forces the subprocess to reload the Python warnings module without the C accelerator module before printing the contents of the filter list.

@ncoghlan
Copy link
Contributor Author

It's also suspicious that ./python -m test test_warnings passed, even though I hadn't fixed this line yet: https://github.com/python/cpython/blob/3.6/Lib/warnings.py#L344 (similar to the changed C code in the PR, those lines assume that the module name is always either None or a compiled regex object - no strings allowed)

@brettcannon
Copy link
Member

It would probably be easier to refactor that code into a function so that you can write a test which tracks what changed in Python prior to launching.

@ncoghlan ncoghlan force-pushed the issue-31975-main-DeprecationWarning-filter branch from 2e6f917 to 661cc5c Compare December 6, 2017 08:37
@ncoghlan ncoghlan force-pushed the issue-31975-main-DeprecationWarning-filter branch from 661cc5c to 7210640 Compare December 30, 2017 04:48
@ncoghlan ncoghlan changed the title bpo-31875 (PEP 565): Show DeprecationWarning in __main__ bpo-31975 (PEP 565): Show DeprecationWarning in __main__ Dec 30, 2017
- primary change is to add a new default filter entry for
  'default::DeprecationWarning:__main__'
- secondary change is an internal one to cope with plain
  strings in the warning module's internal filter list
  (this avoids the need to create a compiled regex object
  early on during interpreter startup)
While CPython doesn't implicitly import `warnings` at startup,
some other implementations might do so. Explicitly clearing the
module cache ensures the subprocess also gets the pure Python
version, no matter what.
@ncoghlan ncoghlan force-pushed the issue-31975-main-DeprecationWarning-filter branch from d1dbf6d to 39200c3 Compare December 30, 2017 05:02
@ncoghlan
Copy link
Contributor Author

I worked out why the new test was passing even though I hadn't updated the pure Python implementation: the pure Python version wasn't configuring the new __main__ filter at all, and the tests were instead checking the C accelerated implementation twice. So I've tweaked that test case to make sure that the subprocess uses the pure Python implementation when that's the version being tested (unlike the C accelerated version, it uses a regex like normal, since it uses the public module API to add the filters). I've also updated the test case to account for the empty default filter list in Py_DEBUG builds.

The PR has been rebased on the current master branch, and I've fixed the issue reference to point to the right place.

@ncoghlan
Copy link
Contributor Author

The Windows failure looks like it's just due to the fact I'm not using universal newlines mode when reading data from the subprocess, so the Windows line endings are confusing matters.

@ncoghlan
Copy link
Contributor Author

ncoghlan commented Jan 5, 2018

@vstinner @brettcannon OK, I think this should be ready for review now - I think I've covered all the doc updates I proposed in the PEP (with a few other cleanups along the way), and the last commit will hopefully fix the line ending problem on Windows.

@ncoghlan ncoghlan merged commit 9b99747 into python:master Jan 8, 2018
@ncoghlan ncoghlan deleted the issue-31975-main-DeprecationWarning-filter branch March 30, 2018 07:52
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

4 participants