Skip to content

gh-85453: Adapt datetime.rst to devguide recommendations for code snippets and variables - #118068

Merged
erlend-aasland merged 17 commits into
python:mainfrom
uatach:wip
Apr 24, 2024
Merged

gh-85453: Adapt datetime.rst to devguide recommendations for code snippets and variables#118068
erlend-aasland merged 17 commits into
python:mainfrom
uatach:wip

Conversation

@uatach

@uatachuatach commented Apr 18, 2024

Copy link
Copy Markdown
Contributor

Pull request based on PR #21447 that was closed due to being too big, I've added only the changes related to backquotes in this PR.


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

@ghost

ghost commented Apr 18, 2024

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

@JelleZijlstraJelleZijlstra 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.

Most of those changes seem unnecessary. In particular, simple integers don't need code formatting.

Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst
Comment threadDoc/library/datetime.rst

@erlend-aaslanderlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO, this PR falls into the same trap as #21447 did. I suggest to follow my advice when I closed that PR: implement the improvements as a series of small changes, one kind of change per PR. Resist the temptation to add any unrelated change. See also Diátaxis, our documentation north star, for more information/inspiration.

IMO, this PR should perform one kind of change only:

Mark up code samples, variables and literals with double backquotes.

This implies: do not add other types of markup such as pairs of * or rephrasings.

Follow up with a PR that adds missing :attr: and :class: markups. If needed, follow up with a PR that consistently marks up parameter names using pairs of *.

Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
@uatachuatach changed the title gh-85453: Improve backquotes, italics and timedelta read-only instance attributesgh-85453: Improve backquotes to follow devguideApr 22, 2024
@erlend-aasland

Copy link
Copy Markdown
Contributor

For future reference, @uatach: we try to avoid force-pushes on pull requests. In order to sync with main, instead do:

$ git switch main
$ git pull
$ git switch wip
$ git merge --no-ff main -m "Pull in main"

See also the devguide :)

@erlend-aaslanderlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some comments:

  • L323-324, L586-587, and L589 should also be updated
  • Please address Jelle's comment: https://github.com/python/cpython/pull/118068/files#r1571709802
  • I think the formatting tables (L2400 and onwards) should be left alone (perhaps they are worth their own PR); the changes made unfortunately seem to make the markup in those tables more inconsistent.

Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
@erlend-aaslanderlend-aasland changed the title gh-85453: Improve backquotes to follow devguidegh-85453: Adapt datetime.rst to devguide recommendations for code snippets and literalsApr 23, 2024
@uatach

Copy link
Copy Markdown
ContributorAuthor

@erlend-aasland as I address the comments, should I also resolve the conversations?

@serhiy-storchakaserhiy-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.

I am not sure that the code formatting should be used for all numbers. Perhaps a variant without this could be simpler to review.

I am not sure that the code formatting instead of italic should be used for references to variables.

Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst
Comment threadDoc/library/datetime.rst
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst
@erlend-aasland

Copy link
Copy Markdown
Contributor

@serhiy-storchaka:

I am not sure that the code formatting instead of italic should be used for references to variables.

This is our recommended style as per the devguide.

Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst Outdated
@uatach

Copy link
Copy Markdown
ContributorAuthor

I have made the requested changes; please review again

@bedevere-app

Copy link
Copy Markdown

Thanks for making the requested changes!

@erlend-aasland, @JelleZijlstra: please review the changes made to this pull request.

Comment threadDoc/library/datetime.rst
Comment threadDoc/library/datetime.rst Outdated
Comment threadDoc/library/datetime.rst
Comment threadDoc/library/datetime.rst Outdated
@erlend-aasland

Copy link
Copy Markdown
Contributor

Thank you so much for picking this up, @uatach! Sorry if this turned out an unpleasant review experience for you; thank you for enduring all the nitpicking!

Feel free to create a PR for any of the remaining items; they should be easier to review, and hopefully you'll get a better contribution experience :)

@erlend-aaslanderlend-aasland changed the title gh-85453: Adapt datetime.rst to devguide recommendations for code snippets and literalsgh-85453: Adapt datetime.rst to devguide recommendations for code snippets and variablesApr 24, 2024
Comment threadDoc/library/datetime.rst Outdated
@erlend-aasland
erlend-aasland enabled auto-merge (squash) April 24, 2024 19:39
@erlend-aasland
erlend-aasland merged commit 809aa9a into python:mainApr 24, 2024
@erlend-aaslanderlend-aasland added the needs backport to 3.12 only security fixes label Apr 24, 2024
@miss-islington-app

Copy link
Copy Markdown

Thanks @uatach for the PR, and @erlend-aasland for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@bedevere-app

Copy link
Copy Markdown

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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docsDocumentation in the Doc dirskip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@uatach@erlend-aasland@JelleZijlstra@serhiy-storchaka