Skip to content

Conversation

ncoghlan
Copy link
Contributor

@ncoghlan ncoghlan commented Dec 6, 2017

Rather than supporting dev mode and the command
line BytesWarning flag directly in the warnings module,
this instead adjusts the initialisation code to add extra
entries to sys.warnoptions when these options are
enabled.

This ensures that dev mode behaves exactly
as if -Wdefault had been passed on the
command line, including in the way it interacts
with sys.warnoptions, and with other command
line flags like -bb.

It also ensures that the -b and -bb options
take precedence of other warnings filter options
like -Wdefault or PYTHONWARNINGS=ignore.

https://bugs.python.org/issue32230

https://bugs.python.org/issue20361

Rather than supporting dev mode directly in
the warnings module, this instead adjusts
the initialisation code to add an extra
'default' entry to sys.warnoptions when
dev mode is enabled.

This ensures that dev mode behaves *exactly*
as if `-Wdefault` had been passed on the
command line, including in the way it interacts
with `sys.warnoptions`, and with other command
line flags like `-bb`.
@ncoghlan ncoghlan requested a review from vstinner December 6, 2017 10:30

out = self.run_xdev("-c", code)
self.assertEqual(out,
"default::Warning "
Copy link
Member

Choose a reason for hiding this comment

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

default::Warning must come after BytesWarning. See also the discussion on https://bugs.python.org/issue32230

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a comment pointing out that Py_BytesWarningFlag means this filter is pretty redundant when neither -b nor -bb is passed.

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

As a command line setting, dev mode should take precedence
of the PYTHONWARNINGS environment variable.
@ncoghlan
Copy link
Contributor Author

ncoghlan commented Dec 6, 2017

89960ef explains why the relative order of the "ignore" BytesWarning filter doesn't really matter in practice.

As mentioned in https://bugs.python.org/issue32231, I'd actually like to start leaving it out entirely in cases where it doesn't matter.

@vstinner
Copy link
Member

vstinner commented Dec 6, 2017

I would prefer to also fix https://bugs.python.org/issue20361 in this PR to fix all warnings options priority at once:
https://bugs.python.org/issue32230#msg307732

Otherwise, I expect a temporary regression until https://bugs.python.org/issue20361 is fixed.

@ncoghlan
Copy link
Contributor Author

ncoghlan commented Dec 6, 2017

Knows issues still to be resolved with the patch:

  • adjust the BytesWarning filter priority as well (and link to the relevant issues from the PR)
  • add a What's New entry describing the change
  • add some more tests to confirm the priority order works as we expect
  • revert some of the changes I made to the implicit filter set in Py_DEBUG builds

- expanded PR to also fix bpo-20361 and have -b & -bb
  take precedence over any other warnings options
- reverted to the short implicit filter list in Py_DEBUG builds
  (this is now even shorter than it was previously, as there
  are *no* implicit filters in debug builds)
@ncoghlan ncoghlan changed the title bpo-32230: Set sys.warnoptions for -X dev bpo-32230, bpo-20361: Set sys.warnoptions for all command line options Dec 7, 2017
@ncoghlan
Copy link
Contributor Author

ncoghlan commented Dec 7, 2017

I have made the requested changes; please review again.

(The one thing I haven't added yet is more test cases, so I'd appreciate suggestions for those)

@bedevere-bot
Copy link

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

@ncoghlan
Copy link
Contributor Author

ncoghlan commented Dec 7, 2017

@vstinner Test case added that checks the warnings filter precedence.

@csabella
Copy link
Contributor

@ncoghlan Should the change be added to the documentation in Doc/c-api/init.rst?

@ncoghlan
Copy link
Contributor Author

ncoghlan commented Dec 12, 2017 via email

@vstinner
Copy link
Member

Since I cannot modify this PR, I created the PR #4820. Nick told me that he is ok if I modify his PR directly, but I'm not allowed to push into his fork of CPython. I squashed his changes and rebase then on master, I fix most test failures (except of test_semaphore_tracker of test_multiprocessing, but this one is not a regression) when using "PYTHONWARNINGS=error ./python -W error -m test -j0 -r", and I updated -X dev documentation.

@vstinner
Copy link
Member

I merged my modified version, PR #4820.

@vstinner vstinner closed this Dec 12, 2017
@ncoghlan
Copy link
Contributor Author

ncoghlan commented Dec 14, 2017 via email

@ncoghlan ncoghlan deleted the issue-32230-set-warnoptions-in-dev-mode branch March 30, 2018 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants