gh-103925: csv.py _guess_quote_and_delimiter should be able to handle windows \r#103926
Conversation
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
|
All commit authors signed the Contributor License Agreement. |
| r'(?:^|\n)(?P<quote>["\']).*?(?P=quote)(?P<delim>[^\w\n"\'])(?P<space> ?)', # ".*?", | ||
| r'(?P<delim>[^\w\n"\'])(?P<space> ?)(?P<quote>["\']).*?(?P=quote)(?:$|\n)', # ,".*?" | ||
| r'(?:^|\n)(?P<quote>["\']).*?(?P=quote)(?:$|\n)'): # ".*?" (no delim, no space) | ||
| for restr in (r'(?P<delim>[^\w\n"\'])(?P<space> ?)(?P<quote>["\']).*?(?P=quote)(?P=delim)', # ,".*?", |
There was a problem hiding this comment.
I think the space rule here is to "align the comments but make the lines as short as possible", which basically means align to the longest line. Now with your change, the third line becomes the longest, so you should probably leave one space between the comment and the code on the third line, and align with that. (one extra space on the other lines if I counted correctly)
There was a problem hiding this comment.
PEP 8: E261 at least two spaces before inline comment
I made another commit to use 2 spaces for now
There was a problem hiding this comment.
Right, two definitely works. The original code uses a single space, probably because the line was long. But two is good for me.
|
This PR is stale because it has been open for 30 days with no activity. |
Conflict in Lib/test/test_csv.py: sample15 was taken by another test in the meantime. Rename the sample added here to sample19 and keep the assertions of both sides. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Describe the user visible effect instead of the implementation, and do not reference a private function which cannot be linked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| @@ -0,0 +1 @@ | |||
| support "\\r" in :func:`_guess_quote_and_delimiter` | |||
There was a problem hiding this comment.
The NEWS entry should talk about user visible API and behavior, not about private functions.
serhiy-storchaka
added
needs backport to 3.13
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
I updated the branch, resolved conflicts with main, and rewrote NEWS.
Now it LGTM.
|
Thanks @lilaboc for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15. |
|
GH-154322 is a backport of this pull request to the 3.15 branch. |
bedevere-app
Bot
removed
the
needs backport to 3.15
|
Sorry, @lilaboc and @serhiy-storchaka, I could not cleanly backport this to |
|
GH-154323 is a backport of this pull request to the 3.14 branch. |
|
GH-154330 is a backport of this pull request to the 3.13 branch. |
…ne (GH-103926) (GH-154323) "$" does not match before "\r" even in the MULTILINE mode, so such a field was not found and the delimiter was guessed from character frequencies instead, which could give a letter. (cherry picked from commit 70f7c6c) Co-authored-by: Zhou Wei <lilaboc.cn@gmail.com> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ne (GH-103926) (GH-154322) "$" does not match before "\r" even in the MULTILINE mode, so such a field was not found and the delimiter was guessed from character frequencies instead, which could give a letter. (cherry picked from commit 70f7c6c) Co-authored-by: Zhou Wei <lilaboc.cn@gmail.com> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 participants
Uh oh!
There was an error while loading. Please reload this page.