◐ Shell
reader mode source ↗
Skip to content

gh-101100: Fix references in csv docs#114658

Merged
serhiy-storchaka merged 5 commits into
python:mainfrom
smontanaro:doc-fix
Jan 30, 2024
Merged

gh-101100: Fix references in csv docs#114658
serhiy-storchaka merged 5 commits into
python:mainfrom
smontanaro:doc-fix

Conversation

@smontanaro

@smontanaro smontanaro commented Jan 27, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

This is my first attempt at this. Hopefully I've done it correctly. I would appreciate checks on my changes and the mechanics (is gh-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 the Dialect documentation reflecting that it really doesn't have any callable methods, and – in particular – lacks a validate method. (Validation is implicit when instantiating the class.)

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

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

@bedevere-app bedevere-app Bot added awaiting review docs Documentation in the Doc dir skip news labels Jan 27, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Jan 27, 2024
Open
hugovk
hugovk reviewed Jan 27, 2024
View reviewed changes

@hugovk hugovk left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

Please remove from Doc/tools/.nitignore

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

@hugovk hugovk added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jan 27, 2024
smontanaro and others added 3 commits January 27, 2024 14:30
@smontanaro
got the seal of approval I guess
601b095
@smontanaro @hugovk
Apply suggestions from code review …
8faab33 
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@smontanaro
per hugovk's suggestion
f3bf42a
@smontanaro

smontanaro commented Jan 27, 2024

Copy link
Copy Markdown
Contributor Author

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

hugovk
hugovk approved these changes Jan 28, 2024
View reviewed changes

@hugovk hugovk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Looks good, thanks!

Just want to double check the write.

serhiy-storchaka
serhiy-storchaka approved these changes Jan 28, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM.

Perhaps we can rename csvwriter to writer, but it is a separate issue, and it may have pitfalls.

@smontanaro

smontanaro commented Jan 28, 2024

Copy link
Copy Markdown
Contributor Author

Perhaps we can rename csvwriter to writer, but it is a separate issue, and it may have pitfalls.

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

@serhiy-storchaka

serhiy-storchaka commented Jan 28, 2024

Copy link
Copy Markdown
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 class csvwriter, but actually it is a description of writerow() in unrelated classes _csv.Writer and csv.DictWriter. csv.DictWriter is not a subclass of _csv.Writer or csv.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

hugovk commented Jan 28, 2024

Copy link
Copy Markdown
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?

@serhiy-storchaka serhiy-storchaka enabled auto-merge (squash) January 30, 2024 21:51
@serhiy-storchaka serhiy-storchaka merged commit 3911b42 into python:main Jan 30, 2024
@miss-islington-app

miss-islington-app Bot commented Jan 30, 2024

Copy link
Copy Markdown

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

miss-islington-app Bot commented Jan 30, 2024

Copy link
Copy Markdown

Sorry, @smontanaro and @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 3911b42cc0d404e0eac87fce30b740b08618ff06 3.12

@miss-islington-app

miss-islington-app Bot commented Jan 30, 2024

Copy link
Copy Markdown

Sorry, @smontanaro and @serhiy-storchaka, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 3911b42cc0d404e0eac87fce30b740b08618ff06 3.11

smontanaro added a commit to smontanaro/cpython that referenced this pull request Jan 30, 2024
@smontanaro @hugovk
pythongh-101100: Fix references in csv docs (pythonGH-114658) …
78be88c 
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
(cherry picked from commit 3911b42)
@smontanaro

smontanaro commented Jan 30, 2024

Copy link
Copy Markdown
Contributor Author

@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

zware commented Jan 30, 2024

Copy link
Copy Markdown
Member

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

@smontanaro

smontanaro commented Jan 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

zware commented Jan 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

I don't know if it was you or cherry_picker, but the PRs were based on main rather than their respective 3.12 or 3.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

smontanaro commented Jan 30, 2024

Copy link
Copy Markdown
Contributor Author

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

@serhiy-storchaka serhiy-storchaka changed the title gh-101100: ref fixes for csv doc Jan 31, 2024
@bedevere-app

bedevere-app Bot commented Jan 31, 2024

Copy link
Copy Markdown

GH-114771 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Jan 31, 2024
@bedevere-app

bedevere-app Bot commented Jan 31, 2024

Copy link
Copy Markdown

GH-114773 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.11 only security fixes label Jan 31, 2024
serhiy-storchaka pushed a commit that referenced this pull request Jan 31, 2024
@smontanaro @hugovk
[3.11] gh-101100: Fix references in csv docs (GH-114658) (GH-114773) …
a38bc7c 
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
(cherry picked from commit 3911b42)
serhiy-storchaka pushed a commit that referenced this pull request Jan 31, 2024
@smontanaro @hugovk
[3.12] gh-101100: Fix references in csv docs (GH-114658) (GH-114771) …
bb578f6 
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
(cherry picked from commit 3911b42)
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@smontanaro @hugovk @aisk
pythongh-101100: Fix references in csv docs (pythonGH-114658) …
0a7b75d 
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka

@hugovk hugovk hugovk approved these changes

Assignees

@serhiy-storchaka serhiy-storchaka

Labels

docs Documentation in the Doc dir skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@smontanaro @serhiy-storchaka @hugovk @zware