Skip to content

Make a note about buggy behavior of csv.QUOTE_NOTNULL and csv.QUOTE_STRINGS #116633

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.

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

Closed
Prometheus3375 opened this issue Mar 12, 2024 · 9 comments
Closed
Labels
3.12 only security fixes 3.13 bugs and security fixes docs Documentation in the Doc dir easy

Comments

@Prometheus3375
Copy link
Contributor

Prometheus3375 commented Mar 12, 2024

csv.QUOTE_NOTNULL and csv.QUOTE_STRINGS do not affect csv.reader in 3.12. This behavior is a bug which is fixed in 3.13 (#113732). But the current documentation of 3.12 still states that these constants affect csv.reader. In the discussion of the issue #113732, it was suggested to make a note to inform the users about the problem.

Meanwhile, there is a small formatting issue: the 3rd occurrence of None in the description of these constants does not use inline code formatting as others. This should be addressed in 3.12 and 3.13 docs as well.
image

Linked PRs

@Prometheus3375 Prometheus3375 added the docs Documentation in the Doc dir label Mar 12, 2024
@hugovk hugovk added the easy label Mar 12, 2024
@hugovk
Copy link
Member

hugovk commented Mar 12, 2024

Thanks for the report, would you like to open a PR for this?

@Prometheus3375
Copy link
Contributor Author

Sure, but I have a couple of questions:

  1. Should the note be present in both 3.12 and 3.13 or only 3.12?
  2. There should one common note about these two constants or only one?

@serhiy-storchaka
Copy link
Member

It is not clear to me how it should be resolved. The documentation clearly specifies how QUOTE_NOTNULL and QUOTE_STRINGS should work in the reader. But the code does not implement it -- the PR was merged prematurely, before implementing this part. There are two ways to resolve the conflict:

  1. Consider it as the bug in the implementation, and change the code. We would have only one or two early 3.12 releases with the bug. It was my preference, but it rejected.
  2. Consider it as the bug in the documentation, and change the documentation. Remove the mention of the reader in 3.12 and document the change as a new feature in 3.13. Now there is another issue -- since it is backward incompatible behavior change, following the strict policies, we should revert the change in 3.13, add a FutureWarning for QUOTE_NOTNULL and QUOTE_STRINGS in the reader in 3.13 and 3.14, and only change the behavior in 3.15.

Option 1 only affects the users of 3.12.0 and 3.12.1. Option 2 makes this feature not particularly useful for three versions: 3.12, 3.13 and 3.14.

cc @Yhg1s, @merwok

@merwok merwok added 3.12 only security fixes 3.13 bugs and security fixes labels Mar 12, 2024
@merwok merwok changed the title Make a note about bugged behavior of csv.QUOTE_NOTNULL and csv.QUOTE_STRINGS in 3.12 Make a note about bugged behavior of csv.QUOTE_NOTNULL and csv.QUOTE_STRINGS Mar 12, 2024
@merwok
Copy link
Member

merwok commented Mar 12, 2024

Original message about versions from T. Wouters:

We should consider observable behaviour, rather than documentation, to be leading. Changing the behaviour in 3.12.2 is too disruptive. Patch releases also should not add new warnings. [...] That it doesn't work as documented is a bug we can't fix in 3.12, but we can in 3.13.

That reasoning was convincing to me, treating it as a normal fix for the next feature release.
But if Serhiy really thinks that this is a bigger change that requires notices, maybe we should get the opinion of the release manager or the steering council to make the right choice.

@Prometheus3375
Copy link
Contributor Author

I made PR #117235 which adds a note about the issue. I took my inspiration from a similar case with **= dispatch bug introduced in 3.8 and then fixed in 3.10. Have a look, @merwok @serhiy-storchaka.

Prometheus3375 added a commit to Prometheus3375/cpython that referenced this issue Jul 24, 2024
@merwok
Copy link
Member

merwok commented Oct 30, 2024

There is a PR for 3.12. What should happen for 3.13?

willingc pushed a commit that referenced this issue Oct 31, 2024
…L and csv.QUOTE_STRINGS (GH-117235)

* Add a note about bug

* Properly link constants
@hugovk hugovk changed the title Make a note about bugged behavior of csv.QUOTE_NOTNULL and csv.QUOTE_STRINGS Make a note about buggy behavior of csv.QUOTE_NOTNULL and csv.QUOTE_STRINGS Oct 31, 2024
@hugovk
Copy link
Member

hugovk commented Oct 31, 2024

The PR for 3.12 says "This bug is fixed in Python 3.13" so I don't think we need to do anything for 3.13.

Thanks for the report and fix!

@hugovk hugovk closed this as completed Oct 31, 2024
@merwok
Copy link
Member

merwok commented Nov 5, 2024

Ah, you mean the docs already described the desired behaviour, so we only need a note in 3.12 and people reading 3.13 docs are fine? Then great!

@Prometheus3375
Copy link
Contributor Author

Both options were added in 3.12, but were not completed properly. The intended behavior for reader was restored via #113732, but it was considered too late to apply to 3.12. So, the note I proposed makes sense only in docs for 3.12 as prior 3.12 no such options existed and after 3.12 the behavior is complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 only security fixes 3.13 bugs and security fixes docs Documentation in the Doc dir easy
Projects
None yet
Development

No branches or pull requests

4 participants