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-67230: add quoting rules to csv module by smontanaro · Pull Request #29469 · python/cpython

Conversation

@smontanaro

@smontanaro smontanaro commented Nov 8, 2021 •
edited by miss-islington
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Add two quoting styles for csv dialects.
They will help to work with certain databases in particular.

  • Issue: csv writer needs more quoting rules #67230

Automerge-Triggered-By: GH:merwok

Sorry, something went wrong.

Uh oh!

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

All reactions
@smontanaro
bpo-23041: update proposed changes to csv module.
7e92373
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 8, 2021
@bedevere-bot bedevere-bot added the awaiting review label Nov 8, 2021
@smontanaro

smontanaro commented Nov 8, 2021

Copy link
Copy Markdown
Contributor Author

I looked to try and find the real people behind the BPO accounts "samwyse" and "krypten" but couldn't. I have no idea how to verify at least krypten has signed a CLA.

All reactions

Sorry, something went wrong.

Uh oh!

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

@github-actions

github-actions Bot commented Dec 9, 2021

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 Dec 9, 2021
@ned-deily

ned-deily commented Feb 4, 2022

Copy link
Copy Markdown
Member

I looked to try and find the real people behind the BPO accounts "samwyse" and "krypten" but couldn't. I have no idea how to verify at least krypten has signed a CLA.

@smontanaro, FWIW, according to the BPO user database, both of those users are registered as having signed the PSF CLA.

All reactions

Sorry, something went wrong.

Uh oh!

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

@MaxwellDupre

MaxwellDupre commented Mar 1, 2022

Copy link
Copy Markdown
Contributor

You don't have empty quote '' in either of your tests:
self._write_test(['a',None,1], '"a",,1', quoting = csv.QUOTE_STRINGS)

self._write_test(['a',None,1], '"a",,"1"', quoting = csv.QUOTE_NOTNULL)

Hence I don't know if the code change produces the correct result.
I note that you can transpose the QUOTE_STRINGS and QUOTE_NOTNULL in these test and the affect is the same, and to be obvious you would only need one!
Also, since this is a little confusing (as to what is expected) could you add an example to the docs, please?
For each new quote so that users can see clearly whats expected and how they differ.

All reactions

Sorry, something went wrong.

Uh oh!

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

@samwyse

samwyse commented May 3, 2022

Copy link
Copy Markdown
Contributor

@smontanaro, Is the CLA Signing waiting on me?

All reactions

Sorry, something went wrong.

Uh oh!

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

@AlexWaygood AlexWaygood closed this May 3, 2022
@AlexWaygood AlexWaygood reopened this May 3, 2022
@AlexWaygood

AlexWaygood commented May 3, 2022

Copy link
Copy Markdown
Member

@smontanaro, Is the CLA Signing waiting on me?

Nope, all good, it just needed a new CI run to be retriggered, which I've just done by closing and reopening the PR :)

All reactions

Sorry, something went wrong.

Uh oh!

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

@samwyse samwyse mannequin mentioned this pull request Apr 10, 2022
csv writer needs more quoting rules #67230
Closed
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@smontanaro
Merge branch 'main' into fix_issue_23041
c9bac12
@AlexWaygood AlexWaygood changed the title bpo-23041: update proposed changes to csv module. gh-67230: update proposed changes to csv module. Nov 1, 2022
@smontanaro
Merge branch 'main' into fix_issue_23041
aadb1d6
@smontanaro

smontanaro commented Nov 3, 2022

Copy link
Copy Markdown
Contributor Author

We need to fish or cut bait on this PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

smontanaro added 3 commits November 4, 2022 07:14
@smontanaro
Merge branch 'main' into fix_issue_23041
9ce98cc
@smontanaro
Merge branch 'main' into fix_issue_23041
93306e1
@smontanaro
Merge branch 'main' into fix_issue_23041
fd97c8d
@msetina

msetina commented Apr 9, 2023

Copy link
Copy Markdown

Why is merging blocked. These quoting types are essential for Microsoft warehousing CSV loading. Is there anyone form MS to push this to get better support for their loaders. CSV is broad format and these quoting types would fill some holes for special loaders without enough parameters to allow CSV prepared with Python.

All reactions

Sorry, something went wrong.

Uh oh!

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

@arhadthedev

arhadthedev commented Apr 9, 2023

Copy link
Copy Markdown
Member

Why is merging blocked

Because only core developers can merge, and the one responsible for the csv module seems to go on vacation.

All reactions

Sorry, something went wrong.

Uh oh!

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

@msetina

msetina commented Apr 10, 2023

Copy link
Copy Markdown

Is there any chance for this to land in 3.12?
I can elaborate why these additional quoting is needed:
As Python has a special value None It would be nice to have capability to control serialization of this to CSV. Currently it serializes into empty string quoted or not. But in some string columns/fields it is required on the parsing side that None is repesented differently than an empty string. If the reception side does not support parsing a special character set into None and it expects empty field without quoting even if field is a string and is quoted. This case is currently not supported in quoting possibilities, but is possible with new quoting rule. All of this is in realm of the RFC that should be supported by CSV module.
I mentioned Microsoft for this case as It is Synapse that has this kind of parser. Without additional postprocessing Synapse will not load CSV produced by Python CSV module. When working with Synapse team they stand by the RFC that their parser supports. I did express a whish to them that they support this change request, but...
I do not understand why it was not added in 3.11.

All reactions

Sorry, something went wrong.

Uh oh!

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

@smontanaro
Merge branch 'main' into fix_issue_23041
ca90711
@smontanaro

smontanaro commented Apr 10, 2023

Copy link
Copy Markdown
Contributor Author

Sorry @msetina. All I can do is update the PR. I'll do what I can in that regard, but someone else will have to push to get it merged. @Mariatta do you have any suggestions?

All reactions

Sorry, something went wrong.

Uh oh!

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

@smontanaro
Merge branch 'main' into fix_issue_23041
e8c2a54
@samwyse

samwyse commented Apr 11, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

I realize that this is over a year old, but only noticed it today...

You don't have empty quote '' in either of your tests: self._write_test(['a',None,1], '"a",,1', quoting = csv.QUOTE_STRINGS)

self._write_test(['a',None,1], '"a",,"1"', quoting = csv.QUOTE_NOTNULL)

Change the test_csv.py to the following:

       self._write_test(['a','',None,1], '"a","",,1',
                        quoting = csv.QUOTE_STRINGS)
       self._write_test(['a','',None,1], '"a","",,"1"',
                        quoting = csv.QUOTE_NOTNULL)

I note that you can transpose the QUOTE_STRINGS and QUOTE_NOTNULL in these test and the affect is the same, and to be obvious you would only need one!

Not so! They differ in the treatment of numeric values. QUOTE_STRINGS would not quote numbers, so 1 is output as 1, while with QUOTE_NOTNULL it is output as "1".

Also, since this is a little confusing (as to what is expected) could you add an example to the docs, please? For each new quote so that users can see clearly whats expected and how they differ.

Let me see what I can do.

All reactions

Sorry, something went wrong.

Uh oh!

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

samwyse added 2 commits April 11, 2023 10:02
@samwyse
Update test_csv.py
54df6b8
Improve tests of QUOTE_STRINGS and QUOTE_NOTNULL
@samwyse
Update csv.rst
d233781
Improve descriptions of QUOTE_NOTNULL and QUOTE_STRINGS with writer objects. Add descriptions for reader objects.
@smontanaro
Merge pull request #39 from samwyse/fix_issue_23041
7beb521
Fix issue 23041
merwok
merwok reviewed Apr 12, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2021-11-07-15-31-25.bpo-23041.564i32.rst Resolved

Uh oh!

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

@merwok
wrap line
4d03980
merwok
merwok reviewed Apr 12, 2023
View reviewed changes
Comment thread Doc/library/csv.rst Resolved

Uh oh!

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

@merwok
fix markup
ba74857
merwok
merwok approved these changes Apr 12, 2023
View reviewed changes
@merwok merwok added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 12, 2023
@bedevere-bot

bedevere-bot commented Apr 12, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @merwok for commit ba74857 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 12, 2023
@merwok
Merge branch 'main' into fix_issue_23041
619dbfa
@merwok merwok added the 🤖 automerge label Apr 12, 2023
@miss-islington

miss-islington commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

@smontanaro: Status check is done, and it's a failure or timed out ❌.

All reactions

Sorry, something went wrong.

Uh oh!

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

@merwok merwok removed the stale Stale PR or inactive for long period of time. label Apr 12, 2023
@merwok merwok closed this Apr 12, 2023
@merwok merwok reopened this Apr 12, 2023
@miss-islington

miss-islington commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

@smontanaro: Status check is done, and it's a failure or timed out ❌.

All reactions

Sorry, something went wrong.

Uh oh!

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

1 similar comment
@miss-islington

miss-islington commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

@smontanaro: Status check is done, and it's a failure or timed out ❌.

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington

miss-islington commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

Status check is done, and it's a success ✅.

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington
miss-islington merged commit 330a942 into python:main Apr 12, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Apr 12, 2023
aisk pushed a commit to aisk/cpython that referenced this pull request Apr 18, 2023
@smontanaro @aisk
pythongh-67230: add quoting rules to csv module (pythonGH-29469)
fb5c807
Add two quoting styles for csv dialects.
They will help to work with certain databases in particular.

Automerge-Triggered-By: GH:merwok
@smontanaro
smontanaro deleted the fix_issue_23041 branch January 18, 2024 13:25
GPHemsley-RELX
GPHemsley-RELX reviewed Jan 31, 2024
View reviewed changes
Comment thread Doc/library/csv.rst Resolved

Uh oh!

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

smontanaro added a commit to smontanaro/cpython that referenced this pull request Jan 31, 2024
@smontanaro
Add versionadded notes for QUOTE_NOTNULL and QUOTE_STRINGS
3056f49
@GPHemsley pointed out that with the recent changes in python#29469, two new data items failed to get `versionadded` notes.
@smontanaro smontanaro mentioned this pull request Jan 31, 2024
gh-67230: Add versionadded notes for QUOTE_NOTNULL and QUOTE_STRINGS #114816
Merged
terryjreedy pushed a commit that referenced this pull request Feb 1, 2024
@smontanaro
gh-67230: Add versionadded notes for QUOTE_NOTNULL and QUOTE_STRINGS (#…
586057e
…114816)

As @GPHemsley pointed out, #29469 omitted `versionadded` notes for the 2 new items.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 1, 2024
@smontanaro @miss-islington
pythongh-67230: Add versionadded notes for QUOTE_NOTNULL and QUOTE_ST…
954ad15
…RINGS (pythonGH-114816)

As @GPHemsley pointed out, pythonGH-29469 omitted `versionadded` notes for the 2 new items.
(cherry picked from commit 586057e)

Co-authored-by: Skip Montanaro <[email protected]>
@miss-islington miss-islington mentioned this pull request Feb 1, 2024
[3.12] gh-67230: Add versionadded notes for QUOTE_NOTNULL and QUOTE_STRINGS (GH-114816) #114840
Merged
terryjreedy pushed a commit that referenced this pull request Feb 1, 2024
@miss-islington @smontanaro
[3.12] gh-67230: Add versionadded notes for QUOTE_NOTNULL and QUOTE_S…
c26deca
…TRINGS (GH-114816) (#114840)

As @GPHemsley pointed out, GH-29469 omitted `versionadded` notes for the 2 new items.
(cherry picked from commit 586057e)

Co-authored-by: Skip Montanaro <[email protected]>
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@smontanaro @aisk
pythongh-67230: Add versionadded notes for QUOTE_NOTNULL and QUOTE_ST…
1bb3a32
…RINGS (python#114816)

As @GPHemsley pointed out, python#29469 omitted `versionadded` notes for the 2 new items.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok approved these changes
+2 more reviewers
@samwyse samwyse samwyse left review comments
@GPHemsley-RELX GPHemsley-RELX GPHemsley-RELX left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

3.12 only security fixes type-feature A feature request or enhancement

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.

13 participants

@smontanaro @ned-deily @MaxwellDupre @samwyse @AlexWaygood @msetina @arhadthedev @bedevere-bot @miss-islington @merwok @GPHemsley-RELX @the-knights-who-say-ni @ezio-melotti
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.