Skip to content

Conversation

@jake-jake-jake
Copy link
Contributor

Looks like there is an extra > in the regex pattern for this sort of csv line: # ,".*?"

Looks like there is an extra > in the regex pattern for this sort of csv line: `# ,".*?"`
@mention-bot
Copy link

@jake-jake-jake, 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
Copy link

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 we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons 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!

@vstinner
Copy link
Member

Please open a bug report to describe the bug and link it to this PR: then add "bpo-xxx" the PR title.

@jake-jake-jake jake-jake-jake changed the title Git rid of extra typo > in sniffer regex pattern for sniffer Git rid of extra typo > in sniffer regex pattern for sniffer bpo-30157 Apr 25, 2017
@jake-jake-jake jake-jake-jake changed the title Git rid of extra typo > in sniffer regex pattern for sniffer bpo-30157 bpo-30157: Get rid of extra typo > in sniffer regex pattern for sniffer Apr 25, 2017
@jake-jake-jake jake-jake-jake changed the title bpo-30157: Get rid of extra typo > in sniffer regex pattern for sniffer bpo-30157: Get rid of extra typo > in sniffer regex pattern for sniff Apr 25, 2017
@jake-jake-jake
Copy link
Contributor Author

@Haypo Let me know if anything else needs adjusting.

@louisom
Copy link
Contributor

louisom commented Apr 25, 2017

Change title to something like bpo-30157: Fix csv sniffer regex patter would be more descriptive.

@serhiy-storchaka
Copy link
Member

I'm surprized that such bug was not caught by tests. This means the lack of tests. Needed a new test for that case.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Needed a test and a Misc/NEWS entry.

@jake-jake-jake jake-jake-jake changed the title bpo-30157: Get rid of extra typo > in sniffer regex pattern for sniff bpo-30157: Fix csv.Sniffer.sniff() regex pattern Apr 25, 2017
@jake-jake-jake
Copy link
Contributor Author

I added some tests for the regex patterns that should ensure that Sniffer._guess_quote_and_delimiter() matches patterns as expected.

@jake-jake-jake
Copy link
Contributor Author

@serhiy-storchaka Let me know if I need to make any more amendments to the tests or news entry; the tests should properly identify when the patterns fail to match.

@serhiy-storchaka
Copy link
Member

Sniffer._guess_quote_and_delimiter() is an implementation detail. Is it possible to write tests for these cases using only public API (Sniffer.sniff())?

@brettcannon
Copy link
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.

@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels Feb 8, 2018
@serhiy-storchaka
Copy link
Member

Recreated as #5601.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting changes type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants