classification
Title: Move test.test_suport.catch_warning() to the 'warnings' module
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.0, Python 2.6
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: benjamin.peterson, brett.cannon, mhammond
Priority: release blocker Keywords: patch

Created on 2008-08-19 18:35 by brett.cannon, last changed 2008-09-02 02:48 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
move_catch_warnings.diff brett.cannon, 2008-08-23 01:19 Move catch_warnings(), update stdlib usage, fix 'line' deprecation check
move_catch_warnings.diff brett.cannon, 2008-08-23 02:20 Updated patch with a minor change
Messages (17)
msg71459 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-08-19 18:35
Since most UNIX distros don't ship Python's test directory,
test.test_support.catch_warning() needs to be moved to the 'warnings'
directory so it can be used to suppress warnings in modules outside the
'test' package.

The default for 'record' will change to False so it is less
test-oriented. The name might change as well since it is more about
saving the state of 'warnings' then actually catching an exception,
although having it return an object representation of a warning makes
the current name make sense.
msg71560 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-08-20 17:24
Just some quick notes on this. The decorator should become:

  def catchwarning(*, record=False, using=warnings)

Name change is to follow the naming convention in 'warnings' (even
though I prefer underscores). The 'using' argument is what module is
being used as the 'warnings' module (mostly just for testing
warings/_warnings). And they are both keyword-only (or at least in
spirit in 2.6) so that the potential to support properly dealing with
__warningregistry__ in the future can be added and the *args arguments
can be those modules to watch.
msg71561 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-08-20 17:29
I don't think the using argument should live in warnings; it's too test
specific. I recommend we still keep a catch_warning in test_support (or
even just test_warnings since this seems to be the only use-case).
msg71577 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-08-20 21:51
On Wed, Aug 20, 2008 at 10:29 AM, Benjamin Peterson
<report@bugs.python.org> wrote:
>
> Benjamin Peterson <musiccomposition@gmail.com> added the comment:
>
> I don't think the using argument should live in warnings; it's too test
> specific. I recommend we still keep a catch_warning in test_support (or
> even just test_warnings since this seems to be the only use-case).
>

But decoupling from the core code of the context manager for this is
not straight-forward without mucking around in sys.modules and that is
always a risky thing to do.
msg71578 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-08-20 21:53
On Wed, Aug 20, 2008 at 4:51 PM, Brett Cannon <report@bugs.python.org> wrote:
>
> Brett Cannon <brett@python.org> added the comment:
>
> On Wed, Aug 20, 2008 at 10:29 AM, Benjamin Peterson
> <report@bugs.python.org> wrote:
>>
>> Benjamin Peterson <musiccomposition@gmail.com> added the comment:
>>
>> I don't think the using argument should live in warnings; it's too test
>> specific. I recommend we still keep a catch_warning in test_support (or
>> even just test_warnings since this seems to be the only use-case).
>>
>
> But decoupling from the core code of the context manager for this is
> not straight-forward without mucking around in sys.modules and that is
> always a risky thing to do.

Why would you have to much around in sys.modules?
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue3602>
> _______________________________________
>

-- 
Cheers,
Benjamin Peterson
"There's no place like 127.0.0.1."
msg71580 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-08-20 22:03
On Wed, Aug 20, 2008 at 2:53 PM, Benjamin Peterson
<report@bugs.python.org> wrote:
>
> Benjamin Peterson <musiccomposition@gmail.com> added the comment:
>
> On Wed, Aug 20, 2008 at 4:51 PM, Brett Cannon <report@bugs.python.org> wrote:
>>
>> Brett Cannon <brett@python.org> added the comment:
>>
>> On Wed, Aug 20, 2008 at 10:29 AM, Benjamin Peterson
>> <report@bugs.python.org> wrote:
>>>
>>> Benjamin Peterson <musiccomposition@gmail.com> added the comment:
>>>
>>> I don't think the using argument should live in warnings; it's too test
>>> specific. I recommend we still keep a catch_warning in test_support (or
>>> even just test_warnings since this seems to be the only use-case).
>>>
>>
>> But decoupling from the core code of the context manager for this is
>> not straight-forward without mucking around in sys.modules and that is
>> always a risky thing to do.
>
> Why would you have to much around in sys.modules?
>>

Well, the bulk of the code needs to live in 'warnings' for it to exist
if the 'test' package is missing. So any differences need to come from
the test.support version. Now the module argument is so that you can
control exactly what module has its showwarnings() implementation
changed without worrying about what 'warnings' is set in sys.modules
and really mucking up the interpreter. But if this argument is missing
then warnings.catchwarnings() will have to set warnings.showwarnings()
blindly since it doesn't know what module object is being tested. So
if I want that change to happen on another module, I need to change
what module is in sys.modules, call the context manager, and then put
it all back so that what I want happen occurs.

That's why you would have to mess with sys.modules. =) The argument
could be renamed '_using', but that just seems silly. And with it
being considered keyword-only (and I will make it the last argument
listed, then most people won't ever run into it.
msg71581 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-08-20 22:05
On Wed, Aug 20, 2008 at 5:03 PM, Brett Cannon <report@bugs.python.org> wrote:
>
> Well, the bulk of the code needs to live in 'warnings' for it to exist
> if the 'test' package is missing. So any differences need to come from
> the test.support version. Now the module argument is so that you can
> control exactly what module has its showwarnings() implementation
> changed without worrying about what 'warnings' is set in sys.modules
> and really mucking up the interpreter. But if this argument is missing
> then warnings.catchwarnings() will have to set warnings.showwarnings()
> blindly since it doesn't know what module object is being tested. So
> if I want that change to happen on another module, I need to change
> what module is in sys.modules, call the context manager, and then put
> it all back so that what I want happen occurs.

Alternatively, you could just have another copy of catch_warning in
test_warnings with the extra argument.
>
> That's why you would have to mess with sys.modules. =) The argument
> could be renamed '_using', but that just seems silly. And with it
> being considered keyword-only (and I will make it the last argument
> listed, then most people won't ever run into it.
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue3602>
> _______________________________________
>
msg71583 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-08-20 22:49
On Wed, Aug 20, 2008 at 3:05 PM, Benjamin Peterson
<report@bugs.python.org> wrote:
>
> Benjamin Peterson <musiccomposition@gmail.com> added the comment:

> Alternatively, you could just have another copy of catch_warning in
> test_warnings with the extra argument.

Sure, but I am not about to support that much code duplication in
Python code when it is purely just to shave off an argument that can
be clearly documented.
msg71585 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-08-20 22:51
All right. You win! :)
msg71778 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-08-22 21:19
I think I can clean up the code by shifting WarningMessage over to a
subclass of namedtuple and moving WarningsRecorder over to a subclass of
list.
msg71798 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-08-23 01:19
The patch does a couple of things. It moves
test.test_support.catch_warning() to warnings.catch_warnings() and
leaves a stub behind. WarningsRecorder becomes a subclass of list to
make it easier to work with. The uses of catch_warnings() in the stdlib
are moved to the 'warnings' implementation along with protecting the
warnings filter changes behind a sys.py3kwarning 'if' statement. And
finally, a oversight in the triggering of a DeprecationWarning for
showwarning() and the new 'line' argument has been fixed (with the
appropriate test).
msg71800 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-08-23 01:47
See my comments on Rietveld. http://codereview.appspot.com/3255
msg71801 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-08-23 02:21
New patch is up with a minor change from Benjamin's review. My explicit
comments on the suggestions are on Rietveld.
msg72059 - (view) Author: Mark Hammond (mhammond) * (Python committer) Date: 2008-08-28 04:26
I stumbled across this when mimetools import of test failed due to
finding a local test package, not the python test package, so I too will
be happy to see this fixed.
msg72252 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-09-01 14:27
I think the patch can now go in.
msg72307 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-09-02 01:28
Applied in the trunk under r66135. Working on merging in 3.0.
msg72311 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-09-02 02:48
r66139 has the 3.0 work. Had to rip out an outdated DeprecationWarning.
Also moved over to keyword-only arguments as stated in the 2.6 docs.
History
Date User Action Args
2008-09-02 02:48:38brett.cannonsetstatus: pending -> closed
resolution: accepted
messages: + msg72311
2008-09-02 01:28:25brett.cannonsetstatus: open -> pending
messages: + msg72307
2008-09-01 20:31:30brett.cannonsetassignee: brett.cannon
2008-09-01 14:27:54benjamin.petersonsetkeywords: - needs review
messages: + msg72252
2008-08-28 04:26:14mhammondsetnosy: + mhammond
messages: + msg72059
2008-08-23 02:21:20brett.cannonsetmessages: + msg71801
2008-08-23 02:20:49brett.cannonsetfiles: + move_catch_warnings.diff
2008-08-23 01:47:00benjamin.petersonsetmessages: + msg71800
2008-08-23 01:20:03brett.cannonsetkeywords: + patch, needs review
assignee: brett.cannon -> (no value)
messages: + msg71798
files: + move_catch_warnings.diff
2008-08-22 21:19:51brett.cannonsetmessages: + msg71778
2008-08-21 18:35:05brett.cannonsetpriority: critical -> release blocker
2008-08-20 22:51:19benjamin.petersonsetmessages: + msg71585
2008-08-20 22:49:22brett.cannonsetmessages: + msg71583
2008-08-20 22:05:31benjamin.petersonsetmessages: + msg71581
2008-08-20 22:03:07brett.cannonsetmessages: + msg71580
2008-08-20 21:53:45benjamin.petersonsetmessages: + msg71578
2008-08-20 21:51:25brett.cannonsetmessages: + msg71577
2008-08-20 17:29:41benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg71561
2008-08-20 17:24:04brett.cannonsetmessages: + msg71560
2008-08-19 18:35:46brett.cannoncreate