GeezerVison

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

GeezerVison is showing github.com — bpo-30825: guess lineterminator in csv.Sniffer by vmax · Pull Request #2529 · python/cpython

Conversation

vmax commented Jul 1, 2017 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

https://bugs.python.org/issue30825

Sorry, something went wrong.

Uh oh!

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

All reactions

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

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!

All reactions

Sorry, something went wrong.

Uh oh!

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

the-knights-who-say-ni added the CLA not signed label

Jul 1, 2017

Mariatta removed the CLA not signed label

Jul 6, 2017

the-knights-who-say-ni added the CLA signed label

Jul 6, 2017

terryjreedy requested changes

Jul 8, 2017

View reviewed changes

terryjreedy 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.

Patch needs tests.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/csv.py Outdated

terryjreedy Jul 8, 2017

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

vmax Jul 8, 2017

Copy link

Copy Markdown

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions


          bpo-30825: guess lineterminator in csv.Sniffer

6f46019

brettcannon added the awaiting changes label

Feb 2, 2018

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

vmax commented Feb 7, 2018

Copy link

Copy Markdown

Author

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot added awaiting change review and removed awaiting changes labels

Feb 7, 2018

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

terryjreedy requested changes

Feb 7, 2018

View reviewed changes

terryjreedy 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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/csv.py


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

terryjreedy Feb 7, 2018

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/csv.py

		if terminator in sample:
		return terminator

		return os.linesep

terryjreedy Feb 7, 2018

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.

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

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 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

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.

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

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

		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

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.

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.

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

		self.assertEqual(dialect.lineterminator, '\n')
		dialect = sniffer.sniff(self.sample10)
		self.assertEqual(dialect.lineterminator, '\r')

terryjreedy Feb 7, 2018

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot added awaiting changes and removed awaiting change review labels

Feb 7, 2018

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.

All reactions

Sorry, something went wrong.

Uh oh!

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


          Fix unit test to cover weird or no terminator case (os.linesep)

524ff18

vmax commented Feb 12, 2018

Copy link

Copy Markdown

Author

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

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

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot added awaiting change review and removed awaiting changes labels

Feb 12, 2018

csabella commented Jan 25, 2020

Copy link

Copy Markdown

Contributor

Closing and reopening to retrigger tests.

All reactions

Sorry, something went wrong.

Uh oh!

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

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

vmax commented Feb 3, 2020

Copy link

Copy Markdown

Author

@csabella replaced with #18336

All reactions

Sorry, something went wrong.

Uh oh!

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

vmax mannequin mentioned this pull request

Apr 10, 2022

csv.Sniffer does not detect lineterminator #75008

Closed

Reviewers

terryjreedy

terryjreedy requested changes

Uh oh!

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

Assignees

No one assigned

Labels

awaiting change review

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.

8 participants

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 01402503 — 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.

Conversation

vmax commented Jul 1, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Jul 1, 2017

Uh oh!

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

Uh oh!

terryjreedy left a comment

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!

brettcannon commented Feb 2, 2018

Uh oh!

vmax commented Feb 7, 2018

Uh oh!

bedevere-bot commented Feb 7, 2018

Uh oh!

terryjreedy left a comment

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!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Feb 7, 2018

Uh oh!

terryjreedy commented Feb 7, 2018

Uh oh!

vmax commented Feb 12, 2018

Uh oh!

bedevere-bot commented Feb 12, 2018

Uh oh!

csabella commented Jan 25, 2020

Uh oh!

csabella commented Jan 25, 2020

Uh oh!

vmax commented Feb 3, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

vmax commented Jul 1, 2017 •

edited by bedevere-bot

Loading

Uh oh!

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