Skip to content
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

bpo-27752: improve documentation of csv.Dialect #26795

Merged
merged 14 commits into from
Aug 6, 2021

Conversation

jdevries3133
Copy link
Contributor

@jdevries3133 jdevries3133 commented Jun 18, 2021

Note that all content changes are in the first commit. The second commit is 100% formatting of the rst file. There are also some very minor proofreading corrections in the final commit.

I'm very open to feedback on this; please let me know if there are any bits in here that you feel could be communicated better!

https://bugs.python.org/issue27752

@iritkatriel
Copy link
Member

Note that all content changes are in the first commit. The second commit is 100% formatting of the rst file. There are also some very minor proofreading corrections in the final commit.

The formatting (whitespace?) changes are unlikely to be approved. You should limit this PR to the content change you are making (possibly fix typos in other places).

The PR will be squashed into one commit, so the diff will be huge and hide the actual change. Sweeping whitespace-type changes are not going to be accepted in a separate PR either because they spam the commit log and the "blame" outputs.

@jdevries3133
Copy link
Contributor Author

@iritkatriel Ok, those changes have been reverted.

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Jul 29, 2021
Copy link
Contributor Author

@jdevries3133 jdevries3133 left a comment

Choose a reason for hiding this comment

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

Don't mind me; just talking to myself. When this was marked stale,I opened it up and saw these issues. I will fix them soon, but don't want anyone to waste their time reviewing in the interim.

Doc/library/csv.rst Outdated Show resolved Hide resolved
Doc/library/csv.rst Outdated Show resolved Hide resolved
Doc/library/csv.rst Outdated Show resolved Hide resolved
Doc/library/csv.rst Outdated Show resolved Hide resolved
@iritkatriel
Copy link
Member

Don't mind me; just talking to myself. When this was marked stale,I opened it up and saw these issues. I will fix them soon, but don't want anyone to waste their time reviewing in the interim.

You can mark the PR as draft and then people won't review it until you say it's ready.

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Jul 30, 2021
@jdevries3133
Copy link
Contributor Author

You can mark the PR as draft and then people won't review it until you say it's ready.

@iritkatriel Yes, well the other factor here is that I was reading the diff just before bed and wanted to leave myself the notes as breadcrumbs, too!

Anyway, this PR is ready for another review, I think it is ready to go. The diff is much smaller as I got rid of all whitespace changes, plus a lot of pedantic changes that I can now see were unnecessary. A summary of the changes now are:

  • fix mechanics in two places: change "the dialect" to "dialects" when speaking about dialect classes, in general
  • revise and expand the paragraph of documentation for the Dialect class to make it more descriptive and less technical-jargon dense
  • create hyperlink by changing change "dialect" to "class:Dialect" where appropriate

Doc/library/csv.rst Outdated Show resolved Hide resolved
Doc/library/csv.rst Outdated Show resolved Hide resolved
jdevries3133 and others added 2 commits August 2, 2021 14:11
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
@ambv ambv added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Aug 6, 2021
@ambv ambv merged commit 0ffdced into python:main Aug 6, 2021
@miss-islington
Copy link
Contributor

Thanks @jdevries3133 for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@bedevere-bot
Copy link

GH-27643 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 6, 2021
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit 0ffdced)

Co-authored-by: Jack DeVries <58614260+jdevries3133@users.noreply.github.com>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Aug 6, 2021
@bedevere-bot
Copy link

GH-27644 is a backport of this pull request to the 3.9 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 6, 2021
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit 0ffdced)

Co-authored-by: Jack DeVries <58614260+jdevries3133@users.noreply.github.com>
miss-islington added a commit that referenced this pull request Aug 6, 2021
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit 0ffdced)

Co-authored-by: Jack DeVries <58614260+jdevries3133@users.noreply.github.com>
ambv pushed a commit that referenced this pull request Aug 6, 2021
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit 0ffdced)

Co-authored-by: Jack DeVries <58614260+jdevries3133@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants