Skip to content

bpo-35178: Ensure custom formatwarning function can receive line as positional argument #12033

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 1, 2019
Merged

bpo-35178: Ensure custom formatwarning function can receive line as positional argument #12033

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6830116
Ensure custom formatwarning function can receive line as positional a…
tirkarthi Feb 25, 2019
5a3f422
Refactor test into a separate test.
tirkarthi Feb 26, 2019
5a018ae
Reword comment
tirkarthi Mar 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions Lib/test/test_warnings/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,6 +877,24 @@ def test_showwarning(self):
file_object, expected_file_line)
self.assertEqual(expect, file_object.getvalue())

def test_formatwarning_override(self):
# bpo-35178: Test custom formatwarning can receive line as positional
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to put the new test in a new method:

def test_formatwarning_override(self):

And maybe move the new test inside test_formatwarning().

Copy link
Member Author

Choose a reason for hiding this comment

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

I initially had it in test_showwarning since the exception was triggered by showwarning. I moved this into a new test test_formatwarning_override. Let me know if I need to move the test into test_formatwarning .

Copy link
Member

Choose a reason for hiding this comment

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

It isn't obvious that the last parameter of myformatwarning() is usually called line, but here it's called "text" to test that the argument is passed as a position argument and not as a keyword argument. Try to rephrase the comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about indicating line could be keyword or positional argument inferring line=line like "Test custom formatwarning can receive line as keyword or positional argument" ? Since line is a common word and also indicates the warning line it's little difficult for me to convey this. Any suggestions would be helpful. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Something like this?

# bpo-35178: Test that a custom formatwarning function gets
# the 'line' argument as a positional argument,
# and not as a keyword argument

(I let you format it properly)

Copy link
Member Author

Choose a reason for hiding this comment

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

I would make a minor edit of "not only as a keyword argument" since this reads a little like line as a keyword argument is not allowed. How about below ?

bpo-35178: Test that a custom formatwarning function gets the 'line' 
argument as a positional argument, and not only as a keyword argument

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's more explicit than the current comment ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Rephrased the comment as noted.

def myformatwarning(message, category, filename, lineno, text):
return f'm={message}:c={category}:f={filename}:l={lineno}:t={text}'

file_name = os.path.splitext(warning_tests.__file__)[0] + '.py'
line_num = 3
file_line = linecache.getline(file_name, line_num).strip()
message = 'msg'
category = Warning
file_object = StringIO()
expected = f'm={message}:c={category}:f={file_name}:l={line_num}' + \
f':t={file_line}'
with support.swap_attr(self.module, 'formatwarning', myformatwarning):
self.module.showwarning(message, category, file_name, line_num,
file_object, file_line)
self.assertEqual(file_object.getvalue(), expected)


class CWarningsDisplayTests(WarningsDisplayTests, unittest.TestCase):
module = c_warnings
Expand Down
2 changes: 1 addition & 1 deletion Lib/warnings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ def _formatwarnmsg(msg):
if fw is not _formatwarning_orig:
# warnings.formatwarning() was replaced
return fw(msg.message, msg.category,
msg.filename, msg.lineno, line=msg.line)
msg.filename, msg.lineno, msg.line)
return _formatwarnmsg_impl(msg)

def filterwarnings(action, message="", category=Warning, module="", lineno=0,
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2019-02-25-23-04-00.bpo-35178.NA_rXa.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Ensure custom :func:`warnings.formatwarning` function can receive `line` as
positional argument. Based on patch by Tashrif Billah.