bpo-35133: Fix mistakes when concatenate string literals on different lines. by serhiy-storchaka · Pull Request #10284 · python/cpython

serhiy-storchaka · 2018-11-01T12:40:34Z

Two kind of mistakes:

Missed space. After concatenating there is no space between words.
Missed comma. Causes unintentional concatenating in a list of strings.

https://bugs.python.org/issue35133

… lines. Two kind of mistakes: 1. Missed space. After concatenating there is no space between words. 2. Missed comma. Causes unintentional concatenating in a list of strings.

ericvsmith

I have a few comments, the others look good. Although this whole change just points out a pitfall of having max. line length constraints: errors like the ones you're fixing are pretty common, and hard to spot.

Lib/idlelib/idle_test/htest.py

ericvsmith · 2018-11-01T12:49:33Z

Lib/test/test_email/test__header_value_parser.py

        group = self._test_get_x(parser.get_group,
-            ('Monty Python:"Fred A. Bear" <dinsdale@example.com>,'
+            ('Monty Python:"Fred A. Bear" <dinsdale@example.com>, '
             'eric@where.test,John <jdoe@test>'),


What about the missing space before John on this and subsequent lines? I realize it's not the issue you're solving, but since you're adding spaces after commas, do you need one here, too? Or maybe the point of this test is to not have spaces after commas? I haven't looked at it closely yet.

These tests is the only thing about which I am unsure.

The email team should look and decide. This again, is structured data, not English prose.

I'm not 100% positive, but I think it's okay in this case. I would wait for comment from @bitdancer though.

I think you should leave all the email tests alone. Having a mixture of spaces after commas and no spaces after commas is good thing in these tests, and although I don't remember for sure, most likely intentional.

Lib/test/test_http_cookiejar.py

Lib/unittest/mock.py

terryjreedy

The fixes for list errors and English misspellings (other than the assert failure message) look correct.

I think an email person (@bitdancer ) should look at the email test data formatting -- or omit the changes.

terryjreedy · 2018-11-01T17:10:31Z

Lib/email/_header_value_parser.py

    if digits[0] == '0' and digits != '0':
-        section.defects.append(errors.InvalidHeaderError("section number"
+        section.defects.append(errors.InvalidHeaderError("section number "
            "has an invalid leading 0"))


section.defects.append(errors.InvalidHeaderError("section number " "has an invalid leading 0"))

could evade the issue and be a bit more readable with

section.defects.append(errors.InvalidHeaderError( "section number has an invalid leading 0"))

Lib/idlelib/idle_test/htest.py

Lib/test/test_email/test__header_value_parser.py

terryjreedy · 2018-11-01T18:08:28Z

Lib/test/test_email/test__header_value_parser.py

        group = self._test_get_x(parser.get_group,
-            ('Monty Python:"Fred A. Bear" <dinsdale@example.com>,'
+            ('Monty Python:"Fred A. Bear" <dinsdale@example.com>, '
             'eric@where.test,John <jdoe@test>'),


The email team should look and decide. This again, is structured data, not English prose.

Lib/test/test_http_cookiejar.py

Lib/test/test_email/test__header_value_parser.py

bitdancer · 2018-11-01T20:11:07Z

Lib/test/test_email/test__header_value_parser.py

        group = self._test_get_x(parser.get_group,
-            ('Monty Python:"Fred A. Bear" <dinsdale@example.com>,'
+            ('Monty Python:"Fred A. Bear" <dinsdale@example.com>, '
             'eric@where.test,John <jdoe@test>'),


I think you should leave all the email tests alone. Having a mixture of spaces after commas and no spaces after commas is good thing in these tests, and although I don't remember for sure, most likely intentional.

bedevere-bot · 2018-11-01T20:12:15Z

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

serhiy-storchaka · 2018-11-01T21:00:57Z

I have made the requested changes; please review again.

bedevere-bot · 2018-11-01T21:01:00Z

Thanks for making the requested changes!

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

asottile

this is amazing, how did you find all of these? is there a tool or was it through some grep?

serhiy-storchaka · 2018-11-05T12:35:10Z

There are tools for multiline searching, but it was easier to me to write a simple Python script.

terryjreedy

The changes requested by myself and David Murray have be made.

The changes to test_email have be made as requested.

terryjreedy · 2018-11-05T14:18:35Z

@bitdancer Since you have not re-reviewed since 44c6760 removed the changes to test_email, 4 days ago, I guessed that you must be busy. So your request was easy to confirm, I took the initiative of doing so in your behalf and dismissing your negative review, and giving Serhiy an OK to merge. If this annoys you, let me know that I should not repeat.

miss-islington · 2018-11-05T14:20:28Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-11-05T14:20:48Z

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

serhiy-storchaka · 2018-11-05T14:20:49Z

Thank you all for your reviews!

miss-islington · 2018-11-05T14:20:49Z

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 34fd4c20198dea6ab2fe8dc6d32d744d9bde868d 3.6

… lines. (pythonGH-10284) Two kind of mistakes: 1. Missed space. After concatenating there is no space between words. 2. Missed comma. Causes unintentional concatenating in a list of strings. (cherry picked from commit 34fd4c2) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

… lines. (GH-10284) Two kind of mistakes: 1. Missed space. After concatenating there is no space between words. 2. Missed comma. Causes unintentional concatenating in a list of strings. (cherry picked from commit 34fd4c2) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…ferent lines. (pythonGH-10284) Two kind of mistakes: 1. Missed space. After concatenating there is no space between words. 2. Missed comma. Causes unintentional concatenating in a list of strings.. (cherry picked from commit 34fd4c2) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

bedevere-bot · 2018-11-05T14:56:07Z

GH-10335 is a backport of this pull request to the 3.6 branch.

…ferent lines. (GH-10284) (GH-10335) Two kind of mistakes: 1. Missed space. After concatenating there is no space between words. 2. Missed comma. Causes unintentional concatenating in a list of strings.. (cherry picked from commit 34fd4c2)

…ferent lines. (pythonGH-10284) (pythonGH-10335) Two kind of mistakes: 1. Missed space. After concatenating there is no space between words. 2. Missed comma. Causes unintentional concatenating in a list of strings.. (cherry picked from commit 34fd4c2). (cherry picked from commit 7054e5c)

…ferent lines. (GH-10284) (GH-10335) (GH-10336) Two kind of mistakes: 1. Missed space. After concatenating there is no space between words. 2. Missed comma. Causes unintentional concatenating in a list of strings. (cherry picked from commit 34fd4c2) (cherry picked from commit 7054e5c)

bpo-35133: Fix mistakes when concatenate string literals on different…

f6c9934

… lines. Two kind of mistakes: 1. Missed space. After concatenating there is no space between words. 2. Missed comma. Causes unintentional concatenating in a list of strings.

serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.6 skip news labels Nov 1, 2018

serhiy-storchaka requested review from a team, rhettinger, skrah and terryjreedy as code owners November 1, 2018 12:40

serhiy-storchaka requested a review from a team November 1, 2018 12:40

the-knights-who-say-ni added the CLA signed label Nov 1, 2018

bedevere-bot added the awaiting merge label Nov 1, 2018

ericvsmith reviewed Nov 1, 2018

View reviewed changes

Remove a dot.

64020f7

tirkarthi reviewed Nov 1, 2018

View reviewed changes

Lib/unittest/mock.py Show resolved Hide resolved

terryjreedy reviewed Nov 1, 2018

View reviewed changes

bitdancer previously requested changes Nov 1, 2018

View reviewed changes

bedevere-bot removed the awaiting merge label Nov 1, 2018

bedevere-bot added the awaiting changes label Nov 1, 2018

serhiy-storchaka added 2 commits November 1, 2018 22:48

Restore email tests.

44c6760

Rewrite _header_value_parser.py as Terry suggested.

2417e92

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 1, 2018

asottile approved these changes Nov 3, 2018

View reviewed changes

terryjreedy approved these changes Nov 5, 2018

View reviewed changes

bedevere-bot removed the awaiting change review label Nov 5, 2018

bedevere-bot added the awaiting merge label Nov 5, 2018

serhiy-storchaka merged commit 34fd4c2 into python:master Nov 5, 2018

bedevere-bot removed the awaiting merge label Nov 5, 2018

serhiy-storchaka deleted the string-literals-concat-multiline branch November 5, 2018 14:20

bedevere-bot removed the needs backport to 3.7 label Nov 5, 2018

miss-islington assigned serhiy-storchaka Nov 5, 2018

bedevere-bot removed the needs backport to 3.6 label Nov 5, 2018

serhiy-storchaka removed their assignment Nov 5, 2018

Uh oh!

Conversation

serhiy-storchaka commented Nov 1, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericvsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 1, 2018

Uh oh!

serhiy-storchaka commented Nov 1, 2018

Uh oh!

bedevere-bot commented Nov 1, 2018

Uh oh!

asottile left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Nov 5, 2018

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

terryjreedy commented Nov 5, 2018

Uh oh!

miss-islington commented Nov 5, 2018

Uh oh!

bedevere-bot commented Nov 5, 2018

Uh oh!

serhiy-storchaka commented Nov 5, 2018

Uh oh!

miss-islington commented Nov 5, 2018

Uh oh!

bedevere-bot commented Nov 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

serhiy-storchaka commented Nov 1, 2018 •

edited by bedevere-bot

Loading