Skip to content

bpo-12178: Fix escaping of escapechar in csv.writer() #13710

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

Jump to bottom
Merged
merged 1 commit into from
Sep 20, 2020
Merged

bpo-12178: Fix escaping of escapechar in csv.writer() #13710

merged 1 commit into from
Sep 20, 2020

Conversation

berkerpeksag
Copy link
Member

@berkerpeksag berkerpeksag commented May 31, 2019 •
edited by bedevere-bot
Loading

taleinat
taleinat reviewed Jun 29, 2019
View reviewed changes
@taleinat
Copy link
Contributor

Since only the quote and escape chars should be escaped, perhaps add a test that other special characters, i.e. delimiters and line terminators, aren't?

taleinat
taleinat approved these changes Jun 29, 2019
View reviewed changes
Copy link
Contributor

@taleinat taleinat left a comment

Choose a reason for hiding this comment

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

LGTM

serhiy-storchaka
serhiy-storchaka reviewed Jul 29, 2019
View reviewed changes
Lib/test/test_csv.py Outdated
(['a/', 1], '"a//",1', csv.QUOTE_MINIMAL),
(['a/', 1], 'a//,1', csv.QUOTE_NONE),
(['a/', 1], '"a//","1"', csv.QUOTE_ALL),
(['a/b', 1], '"a//b",1', csv.QUOTE_MINIMAL),
Copy link
Member

Choose a reason for hiding this comment

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

Are quotes needed here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@serhiy-storchaka, your question is unclear to me.

Copy link
Member

Choose a reason for hiding this comment

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

Why the output is "a//b",1 instead of just a//b,1?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question! I'm wondering the same thing now. Might just be a bug!

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem consistent with the docs:

Instructs writer objects to only quote those fields which contain special characters such as delimiter, quotechar or any of the characters in lineterminator.

Copy link

Choose a reason for hiding this comment

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

Yes, I agree there shouldn't be quotes here. They should be removed from the tests (and implemented correctly) before merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be fixed now. Please take a look again.

@berkerpeksag
Copy link
Member Author

I'm going to take a look at the PR this weekend. Sorry for my late response.

@taleinat
Copy link
Contributor

@berkerpeksag, let me know if there's any way I can help this get into 3.8.

@taleinat
Copy link
Contributor

Ping? We already missed 3.8, let's not miss 3.9!

@berkerpeksag berkerpeksag mentioned this pull request Aug 1, 2020
Closed
@taleinat
Copy link
Contributor

Ping, @berkerpeksag?

@berkerpeksag
Copy link
Member Author

I've already addressed review comments back in July.

@taleinat
Copy link
Contributor

I've already addressed review comments back in July.

Ah, sorry, you're right, I missed that!

taleinat
taleinat approved these changes Sep 19, 2020
View reviewed changes
Copy link
Contributor

@taleinat taleinat left a comment

Choose a reason for hiding this comment

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

LGTM

@taleinat
Copy link
Contributor

Closing and re-opening to re-run the stalled Travis-CI check.

@taleinat taleinat closed this Sep 19, 2020
@taleinat taleinat reopened this Sep 19, 2020
@taleinat taleinat added the type-bug An unexpected behavior, bug, or error label Sep 19, 2020
@taleinat
Copy link
Contributor

@berkerpeksag, what do you think about whether this should be back-ported?

While it's obviously a bug-fix, I'm conflicted since this could break code which relied on the existing behavior.

@berkerpeksag
Copy link
Member Author

While it's obviously a bug-fix, I'm conflicted since this could break code which relied on the existing behavior.

I agree with you. I'm totally fine with either backporting to only 3.9 or no backport.

@taleinat
Copy link
Contributor

3.9.0rc2 is out and only critical fixes should go into 3.9.0, and I wouldn't want this in 3.9.1 but not 3.9.0. So let's avoid back-porting.

@taleinat taleinat merged commit 5c0eed7 into python:master Sep 20, 2020
@berkerpeksag berkerpeksag deleted the bpo-12178-csv branch September 20, 2020 08:55
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
@berkerpeksag @Tapuzi
bpo-12178: Fix escaping of escapechar in csv.writer() (pythonGH-13710)
d311098 
Co-authored-by: Itay Elbirt <[email protected]>
@ebreck ebreck mannequin mentioned this pull request Sep 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justhalf justhalf justhalf left review comments

@taleinat taleinat taleinat approved these changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Assignees

@berkerpeksag berkerpeksag

Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@berkerpeksag @taleinat @justhalf @serhiy-storchaka @gpshead @the-knights-who-say-ni @bedevere-bot