FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-30825: guess lineterminator in csv.Sniffer by vmax · Pull Request #2529 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

bpo-30825: guess lineterminator in csv.Sniffer - #2529

Closed
vmax wants to merge 2 commits into
python:masterfrom
vmax:fix-issue-30825
ConversationCommits (2)ChecksFiles changed
Closed

bpo-30825: guess lineterminator in csv.Sniffer#2529
vmax wants to merge 2 commits into
python:masterfrom
vmax:fix-issue-30825

Conversation

vmax commented Jul 1, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown

mention-bot commented Jul 1, 2017

Copy link
Copy Markdown

@vmax, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rhettinger, @benjaminp and @birkenfeld to be potential reviewers.

the-knights-who-say-ni commented Jul 1, 2017

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

terryjreedy requested changes Jul 8, 2017
View reviewed changes

terryjreedy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Patch needs tests.

Comment thread Lib/csv.py Outdated

terryjreedy Jul 8, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Every '\r\n' will be counted as '\r' and '\n' as well as '\r\n'. So I believe the '\r\n' count should be subtracted from the other 2 before comparing.

vmax Jul 8, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@terryjreedy I changed detection method; most probably line terminators would be consistent so if one of them is present, it would be the one

brettcannon commented Feb 2, 2018

Copy link
Copy Markdown
Member

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

vmax commented Feb 7, 2018

Copy link
Copy Markdown
Author

I have made the requested changes; please review again

bedevere-bot commented Feb 7, 2018

Copy link
Copy Markdown

Thanks for making the requested changes!

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

terryjreedy requested changes Feb 7, 2018
View reviewed changes

terryjreedy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Revise new test as suggested. Subject to more review, the code change looks good to me.

Comment thread Lib/csv.py

for terminator in terminators:
if terminator in sample:
return terminator

terryjreedy Feb 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is the core of the patch. The sequence looks right.

Comment thread Lib/csv.py
if terminator in sample:
return terminator

return os.linesep

terryjreedy Feb 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In case user submits a sample with an unusual or no line terminator.

Comment thread Lib/test/test_csv.py Outdated
'Tommy''s Place'+ Blue Island'+ 'IL'+ '12/28/02'+ 'Blue Sunday/White Crow'
'Stonecutters ''Seafood'' and Chop House'+ 'Lemont'+ 'IL'+ '12/19/02'+ 'Week Back'
"""
sample10 = "Date;Value\r2010-01-01;10\r2010-01-01;20"

terryjreedy Feb 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Either move this line into the test function or put the literal into the sniff call. See comment below.

Comment thread Lib/test/test_csv.py
dialect = sniffer.sniff(self.sample5)
self.assertEqual(dialect.lineterminator, '\r\n')
dialect = sniffer.sniff(self.sample8)
self.assertEqual(dialect.lineterminator, '\n')

terryjreedy Feb 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This assumes that the line terminator for multiline triple-quoted string literals is '\n'. We require that '\r\n' in code files be converted to '\n' before merging, but the test should not assume '\n' for all developer and user systems where the test might be run. So sample8 should be replaced by a literal.

Since the sample strings need not be long, they can all be literals defined here. Make all 3 the same except for the terminator. This will make the test both crystal clear and independent of system and optional local settings.

Comment thread Lib/test/test_csv.py
self.assertEqual(dialect.lineterminator, '\n')
dialect = sniffer.sniff(self.sample10)
self.assertEqual(dialect.lineterminator, '\r')

terryjreedy Feb 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Add tests to cover the last line of _guess_lineterminator: return os.linesep. Add a test with the same literal, but with an oddball terminator, such as '\v' (vertical tab), and another with a single line with no control chars.

bedevere-bot commented Feb 7, 2018

Copy link
Copy Markdown

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.

terryjreedy commented Feb 7, 2018

Copy link
Copy Markdown
Member

If you have installed the blurb module (and you should if you think you might submit other PRs), please add a news file with the next revision.

vmax commented Feb 12, 2018

Copy link
Copy Markdown
Author

I have made the requested changes; please review again

bedevere-bot commented Feb 12, 2018

Copy link
Copy Markdown

Thanks for making the requested changes!

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

csabella commented Jan 25, 2020

Copy link
Copy Markdown
Contributor

Closing and reopening to retrigger tests.

csabella closed this Jan 25, 2020

csabella commented Jan 25, 2020

Copy link
Copy Markdown
Contributor

Oops, I was not able to reopen after closing as this was on an 'unknown repository'. This pull request will need to be recreated as a new PR.

vmax commented Feb 3, 2020

Copy link
Copy Markdown
Author

@csabella replaced with #18336

vmax mannequin mentioned this pull request Apr 10, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

terryjreedy terryjreedy requested changes

Assignees

No one assigned

Labels

awaiting change review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL