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

tirkarthi · 2019-02-25T18:01:39Z

Custom warnings.formatwarning function can receive line as positional argument.

Co-Authored-By: Tashrif Billah tashrifbillah@gmail.com
Co-Authored-By: Xtreak tir.karthi@gmail.com

https://bugs.python.org/issue35178

…rgument. Co-Authored-By: Tashrif Billah <tashrifbillah@gmail.com> Co-Authored-By: Xtreak <tir.karthi@gmail.com>

tirkarthi · 2019-02-25T18:18:04Z

I have added a test with the fix adopted from #10343. Please review.

cc @tashrifbillah @vstinner

vstinner · 2019-02-25T21:24:32Z

Lib/test/test_warnings/__init__.py

@@ -877,6 +877,18 @@ def test_showwarning(self):
                                file_object, expected_file_line)
        self.assertEqual(expect, file_object.getvalue())

+        # bpo-35178: Test custom formatwarning can receive line as positional
+        def formatwarning(message, category, filename, lineno, text):
+            return text


I would prefer to use all arguments in the result, to check that all arguments are passed and in the correct order.

For example: return f'm={message}:c={category}:f={filename}:l={lineno}:t={text}'

Changed it.

vstinner · 2019-02-25T21:25:41Z

Lib/test/test_warnings/__init__.py

+
+        file_object = StringIO()
+        original = self.module.formatwarning
+        self.module.formatwarning = formatwarning


I would prefer to use "with mock.patch.object(self.module, 'formatwarning', formatwarning): ..." or "with support.swap_attr(self.module, 'formatwarning', formatwarning): ..." here, to ensure that the attribute is restored on error.

I initially had self.addCleanup to restore it but your suggestion looks more compact and robust. I used support.swap_attr since support was already imported. Thanks

vstinner · 2019-02-25T21:26:03Z

Lib/test/test_warnings/__init__.py

@@ -877,6 +877,18 @@ def test_showwarning(self):
                                file_object, expected_file_line)
        self.assertEqual(expect, file_object.getvalue())

+        # bpo-35178: Test custom formatwarning can receive line as positional
+        def formatwarning(message, category, filename, lineno, text):


nitpick: use a different function name to avoid confusion, ex: "def myformatwarning"

vstinner · 2019-02-25T21:27:43Z

Lib/test/test_warnings/__init__.py

@@ -877,6 +877,18 @@ def test_showwarning(self):
                                file_object, expected_file_line)
        self.assertEqual(expect, file_object.getvalue())

+        # bpo-35178: Test custom formatwarning can receive line as positional


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().

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 .

bedevere-bot · 2019-02-25T21:28:12Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

Co-Authored-By: Tashrif Billah <tashrifbillah@gmail.com> Co-Authored-By: Xtreak <tir.karthi@gmail.com>

tirkarthi · 2019-02-28T05:43:03Z

I have made the requested changes; please review again

bedevere-bot · 2019-02-28T05:43:06Z

Thanks for making the requested changes!

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

vstinner · 2019-02-28T15:56:01Z

Lib/test/test_warnings/__init__.py

@@ -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


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.

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.

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)

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

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

Rephrased the comment as noted.

miss-islington · 2019-03-01T17:18:39Z

Thanks @tirkarthi for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

Ensure custom formatwarning function can receive line as positional argument. Co-Authored-By: Tashrif Billah <tashrifbillah@gmail.com> (cherry picked from commit be7c460) Co-authored-by: Xtreak <tir.karthi@gmail.com>

bedevere-bot · 2019-03-01T17:18:52Z

GH-12130 is a backport of this pull request to the 3.7 branch.

Ensure custom formatwarning function can receive line as positional argument. Co-Authored-By: Tashrif Billah <tashrifbillah@gmail.com> (cherry picked from commit be7c460) Co-authored-by: Xtreak <tir.karthi@gmail.com>

Ensure custom formatwarning function can receive line as positional a…

6830116

…rgument. Co-Authored-By: Tashrif Billah <tashrifbillah@gmail.com> Co-Authored-By: Xtreak <tir.karthi@gmail.com>

the-knights-who-say-ni added the CLA signed label Feb 25, 2019

bedevere-bot added the awaiting review label Feb 25, 2019

vstinner requested changes Feb 25, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Feb 25, 2019

Refactor test into a separate test.

5a3f422

Co-Authored-By: Tashrif Billah <tashrifbillah@gmail.com> Co-Authored-By: Xtreak <tir.karthi@gmail.com>

bedevere-bot added awaiting change review and removed awaiting changes labels Feb 28, 2019

vstinner reviewed Feb 28, 2019

View reviewed changes

Reword comment

5a018ae

vstinner approved these changes Mar 1, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Mar 1, 2019

vstinner merged commit be7c460 into python:master Mar 1, 2019

bedevere-bot removed the awaiting merge label Mar 1, 2019

vstinner added the needs backport to 3.7 label Mar 1, 2019

bedevere-bot removed the needs backport to 3.7 label Mar 1, 2019

vstinner mentioned this pull request Mar 7, 2019

bpo-35178: Pass positional arguments to formatwarning(). #10343

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tirkarthi commented Feb 25, 2019 •

edited by bedevere-bot

Loading

tirkarthi commented Feb 25, 2019

vstinner Feb 25, 2019

tirkarthi Feb 26, 2019

vstinner Feb 25, 2019

tirkarthi Feb 26, 2019

vstinner Feb 25, 2019

vstinner Feb 25, 2019

tirkarthi Feb 26, 2019

bedevere-bot commented Feb 25, 2019

tirkarthi commented Feb 28, 2019

bedevere-bot commented Feb 28, 2019

vstinner Feb 28, 2019

tirkarthi Feb 28, 2019

vstinner Feb 28, 2019

tirkarthi Feb 28, 2019

vstinner Feb 28, 2019

tirkarthi Mar 1, 2019

miss-islington commented Mar 1, 2019

bedevere-bot commented Mar 1, 2019

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

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

Conversation

tirkarthi commented Feb 25, 2019 • edited by bedevere-bot Loading

tirkarthi commented Feb 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Feb 25, 2019

tirkarthi commented Feb 28, 2019

bedevere-bot commented Feb 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miss-islington commented Mar 1, 2019

bedevere-bot commented Mar 1, 2019

tirkarthi commented Feb 25, 2019 •

edited by bedevere-bot

Loading