Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-101100: Fix references in csv docs#114658

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
serhiy-storchaka merged 5 commits intopython:mainfromsmontanaro:doc-fix
Jan 30, 2024

Conversation

smontanaro
Copy link
Contributor

@smontanarosmontanaro commentedJan 27, 2024•
edited by github-actionsbot
Loading

This is my first attempt at this. Hopefully I've done it correctly. I would appreciate checks on my changes and the mechanics (isgh-101100 the correct issue to reference?). Thanks to@hugovk for helping me get started. There are two types of changes herein:

  1. The usual reference fixes.
  2. A small wording change to theDialect documentation reflecting that it really doesn't have any callable methods, and – in particular – lacks avalidate method. (Validation is implicit when instantiating the class.)

With these changes, nitpicky mode in Sphinx seems happy withcsv.rst.

📚 Documentation preview 📚:https://cpython-previews--114658.org.readthedocs.build/

Copy link
Member

@hugovkhugovk left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Thank you!

is#101100 the correct issue to reference?

Yep :)

With these changes, nitpicky mode in Sphinx seems happy with csv.rst.

And the CI has failed... But in a good way! It says:

Congratulations! You improved:Doc/library/csv.rstPlease remove from Doc/tools/.nitignore

Doing this means the CI will also prevent new warnings from appearing in this file.

@@ -492,9 +492,9 @@ DictReader objects have the following public attribute:
Writer Objects
--------------

:class:`Writer` objects (:class:`DictWriter` instances and objects returned by
:class:`Writer<writer>` objects (:class:`DictWriter` instances and objects returned by
Copy link
Member

Choose a reason for hiding this comment

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

We usually add a space here:

Suggested change
:class:`Writer<writer>` objects (:class:`DictWriter` instances and objects returned by
:class:`Writer<writer>` objects (:class:`DictWriter` instances and objects returned by

Although, I don't see aWriter class with capital W, is it only capitalised because it begins the sentence? If so, I'd either reword so it's not first, or leave it lowercase.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, I left it capitalized. I suspect the original author may have done it that way. I just added a proper reference. Thecsv module dates from the days when types (written in C) and classes (written in Python) were not quite the same thing. Type names generally weren't capitalized (I think, hencecsv.writer), while classes were, hencecsv.DictWriter.

@hugovkhugovk added needs backport to 3.11only security fixes needs backport to 3.12only security fixes labelsJan 27, 2024
smontanaroand others added3 commitsJanuary 27, 2024 14:30
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@smontanaro
Copy link
ContributorAuthor

@hugovk I think I made all the suggested changes...

hugovk reacted with thumbs up emoji

Copy link
Member

@hugovkhugovk left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

Just want to double check thewrite.

@@ -88,7 +88,7 @@ The :mod:`csv` module defines the following functions:

Return a writer object responsible for converting the user's data into delimited
strings on the given file-like object. *csvfile* can be any object with a
:func:`write` method. If *csvfile* is a file object, it should be opened with
:meth:`~io.TextIOBase.write` method. If *csvfile* is a file object, it should be opened with
Copy link
Member

Choose a reason for hiding this comment

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

@serhiy-storchaka Is this the correct one forwrite in this case?

Choose a reason for hiding this comment

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

I guess it is.

hugovk reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Good question. I mulled that over a bit before usingTextIOBase, eventually deciding that since CSV files are supposed to be opened as plain text files since Python 3, that was the most reasonable link destination.

Choose a reason for hiding this comment

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

You can also explicitly mention thatwrite() should accept a string argument (not bytes). And the same for the reader.

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

Perhaps we can renamecsvwriter towriter, but it is a separate issue, and it may have pitfalls.

@@ -88,7 +88,7 @@ The :mod:`csv` module defines the following functions:

Return a writer object responsible for converting the user's data into delimited
strings on the given file-like object. *csvfile* can be any object with a
:func:`write` method. If *csvfile* is a file object, it should be opened with
:meth:`~io.TextIOBase.write` method. If *csvfile* is a file object, it should be opened with

Choose a reason for hiding this comment

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

I guess it is.

hugovk reacted with thumbs up emoji
@smontanaro
Copy link
ContributorAuthor

Perhaps we can renamecsvwriter towriter, but it is a separate issue, and it may have pitfalls.

Since there is no type calledcsvwriter, what pitfalls would correcting those references be?

@serhiy-storchaka
Copy link
Member

csv.writer is not a class,csv.writer() returns an instance of_csv.Writer class which is not exposed itself.writerow() is defined as a method of a fictional classcsvwriter, but actually it is a description ofwriterow() in unrelated classes_csv.Writer andcsv.DictWriter.csv.DictWriter is not a subclass of_csv.Writer orcsv.writer (which is not even a class). All so complicated, that I cannot predict what Sphinx will produce. We have to try and see at the result. All can just work, or produce glitches from which we could not know how to get rid.

@hugovk
Copy link
Member

In the spirit of incremental changes, shall we merge this as-is (it does what it set out to do: fix reference warnings), and make a new PR for any further changes?

smontanaro, hauntsaninja, and serhiy-storchaka reacted with thumbs up emoji

@serhiy-storchakaserhiy-storchakaenabled auto-merge (squash)January 30, 2024 21:51
@serhiy-storchakaserhiy-storchaka merged commit3911b42 intopython:mainJan 30, 2024
@miss-islington-app
Copy link

Thanks@smontanaro for the PR, and@serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry,@smontanaro and@serhiy-storchaka, I could not cleanly backport this to3.12 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker 3911b42cc0d404e0eac87fce30b740b08618ff06 3.12

@miss-islington-app
Copy link

Sorry,@smontanaro and@serhiy-storchaka, I could not cleanly backport this to3.11 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker 3911b42cc0d404e0eac87fce30b740b08618ff06 3.11

smontanaro added a commit to smontanaro/cpython that referenced this pull requestJan 30, 2024
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>(cherry picked from commit3911b42)
@smontanaro
Copy link
ContributorAuthor

@serhiy-storchaka@hugovk I attempted to create the two pull requests. Please take a look and let me know if I missed something or resolved the.nitignore conflict incorrectly (quite possible).

@zware
Copy link
Member

I haven't reviewed the content ofGH-114771 (3.12) orGH-114773 (3.11), but the diffs look a lot better with the right base branches :)

AlexWaygood reacted with thumbs up emoji

@smontanaro
Copy link
ContributorAuthor

smontanaro commentedJan 30, 2024•
edited
Loading

Not sure what you're referring to. I was just executing the cherry-picker commands the bedevere not have. Did I mess something up?

@zware
Copy link
Member

zware commentedJan 30, 2024•
edited
Loading

I don't know if it was you orcherry_picker, but the PRs were based onmain rather than their respective3.12 or3.11 branch.

The PR branches themselves were fine, though. That is, the error was in the creation of the PRs, not the creation of the backports.

@smontanaro
Copy link
ContributorAuthor

That's weird. I did run the commands from my 3.12 and 3.11 repos.

@serhiy-storchakaserhiy-storchaka changed the titlegh-101100: ref fixes for csv docgh-101100: Fix references in csv docsJan 31, 2024
@bedevere-app
Copy link

GH-114771 is a backport of this pull request to the3.12 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.12only security fixes labelJan 31, 2024
@bedevere-app
Copy link

GH-114773 is a backport of this pull request to the3.11 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.11only security fixes labelJan 31, 2024
serhiy-storchaka pushed a commit that referenced this pull requestJan 31, 2024
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>(cherry picked from commit3911b42)
serhiy-storchaka pushed a commit that referenced this pull requestJan 31, 2024
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>(cherry picked from commit3911b42)
aisk pushed a commit to aisk/cpython that referenced this pull requestFeb 11, 2024
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@hugovkhugovkhugovk approved these changes

Labels
docsDocumentation in the Doc dirskip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@smontanaro@serhiy-storchaka@hugovk@zware
[8]ページ先頭
©2009-2025 Movatter.jp