GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-103925: csv.py _guess_quote_and_delimiter should be able to handle windows \r by lilaboc · Pull Request #103926 · python/cpython

Conversation

@lilaboc

@lilaboc lilaboc commented Apr 27, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: csv.py _guess_quote_and_delimiter should be able to handle windows \r #103925

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Samyak2 reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@lilaboc
fix python#103925
4f4ea45
@bedevere-bot

bedevere-bot commented Apr 27, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot mentioned this pull request Apr 27, 2023
csv.py _guess_quote_and_delimiter should be able to handle windows \r #103925
Closed
@bedevere-bot bedevere-bot added the awaiting review label Apr 27, 2023
@ghost

ghost commented Apr 27, 2023 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@blurb-it
📜🤖 Added by blurb_it.
80994d6
gaogaotiantian
gaogaotiantian reviewed Apr 27, 2023
View reviewed changes
Comment thread Lib/csv.py Outdated
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)', # ,".*?",

@gaogaotiantian gaogaotiantian Apr 27, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lilaboc lilaboc Apr 27, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

PEP 8: E261 at least two spaces before inline comment
I made another commit to use 2 spaces for now

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gaogaotiantian gaogaotiantian Apr 27, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Right, two definitely works. The original code uses a single space, probably because the line was long. But two is good for me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_csv.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Library/2023-04-27-14-50-03.gh-issue-103925.khC-El.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
lilaboc added 3 commits April 27, 2023 23:40
@lilaboc
Update 2023-04-27-14-50-03.gh-issue-103925.khC-El.rst
b27806c
@lilaboc
fix python#103925
bd808da
@lilaboc
Merge remote-tracking branch 'origin/fix-issue-103925' into fix-issue…
35e32d3 
…-103925
gaogaotiantian
gaogaotiantian approved these changes Apr 27, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Apr 27, 2023
@lilaboc
Merge branch 'main' into fix-issue-103925
2058440
@github-actions

github-actions Bot commented Apr 12, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 12, 2026
@serhiy-storchaka serhiy-storchaka mentioned this pull request Jul 14, 2026
gh-83273: Rewrite csv.Sniffer dialect detection using trial parsing #153694
Merged
serhiy-storchaka and others added 2 commits July 21, 2026 07:46
@serhiy-storchaka @claude
Merge branch 'main' into fix-issue-103925
ff0e806 
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) <[email protected]>
@serhiy-storchaka @claude
Rewrite the NEWS entry
a54438b 
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) <[email protected]>
serhiy-storchaka
serhiy-storchaka reviewed Jul 21, 2026
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2023-04-27-14-50-03.gh-issue-103925.khC-El.rst Outdated
@@ -0,0 +1 @@
support "\\r" in :func:`_guess_quote_and_delimiter`

@serhiy-storchaka serhiy-storchaka Jul 21, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The NEWS entry should talk about user visible API and behavior, not about private functions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka serhiy-storchaka added needs backport to 3.13 only security fixes needs backport to 3.14 bugs and security fixes needs backport to 3.15 bugs and security fixes labels Jul 21, 2026
serhiy-storchaka
serhiy-storchaka approved these changes Jul 21, 2026
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I updated the branch, resolved conflicts with main, and rewrote NEWS.

Now it LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jul 21, 2026
@serhiy-storchaka
serhiy-storchaka enabled auto-merge (squash) July 21, 2026 04:59
@serhiy-storchaka
serhiy-storchaka merged commit 70f7c6c into python:main Jul 21, 2026
63 checks passed

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app

miss-islington-app Bot commented Jul 21, 2026

Copy link
Copy Markdown

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.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting merge label Jul 21, 2026
@bedevere-app

bedevere-app Bot commented Jul 21, 2026

Copy link
Copy Markdown

GH-154322 is a backport of this pull request to the 3.15 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.15 bugs and security fixes label Jul 21, 2026
@miss-islington-app

miss-islington-app Bot commented Jul 21, 2026

Copy link
Copy Markdown

Sorry, @lilaboc and @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 70f7c6c0f2ddfd3b447946f1b926776b2a344703 3.13
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app miss-islington-app Bot assigned serhiy-storchaka Jul 21, 2026
@bedevere-app

bedevere-app Bot commented Jul 21, 2026

Copy link
Copy Markdown

GH-154323 is a backport of this pull request to the 3.14 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Jul 21, 2026
@bedevere-app

bedevere-app Bot commented Jul 21, 2026

Copy link
Copy Markdown

GH-154330 is a backport of this pull request to the 3.13 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Jul 21, 2026
serhiy-storchaka added a commit that referenced this pull request Jul 21, 2026
@miss-islington @lilaboc
@serhiy-storchaka @claude
[3.14] gh-103925: Fix csv.Sniffer for a quoted field ending a CRLF li…
42459c9 
…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 <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
serhiy-storchaka added a commit that referenced this pull request Jul 21, 2026
@miss-islington @lilaboc
@serhiy-storchaka @claude
[3.15] gh-103925: Fix csv.Sniffer for a quoted field ending a CRLF li…
c0129a0 
…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 <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
serhiy-storchaka added a commit that referenced this pull request Jul 21, 2026
@serhiy-storchaka @lilaboc
[3.13] gh-103925: Fix csv.Sniffer for a quoted field ending a CRLF li…
0bc6595 
…ne (GH-103926) (GH-154330)

"$" 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@gaogaotiantian gaogaotiantian gaogaotiantian approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@serhiy-storchaka serhiy-storchaka

Labels

stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@lilaboc @bedevere-bot @serhiy-storchaka @gaogaotiantian
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.