Skip to content

gh-131912: Refactored description of grouping options in format specification docs #132030

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Apr 5, 2025
Merged

gh-131912: Refactored description of grouping options in format specification docs #132030

merged 11 commits into from
Apr 5, 2025

Conversation

Prometheus3375
Copy link
Contributor

@Prometheus3375 Prometheus3375 commented Apr 2, 2025 •
edited
Loading

This PR refactors descriptions for grouping options. It places it after width and precision where grouping can be used. It also fixes two typos: (a) grouping wasn't optional for precision and (b) sentences for sign options were not capitalized.

📚 Documentation preview 📚

@bedevere-app bedevere-app bot added awaiting review docs Documentation in the Doc dir skip news labels Apr 2, 2025
@github-project-automation github-project-automation bot moved this to Todo in Docs PRs Apr 2, 2025
@bedevere-app bedevere-app bot mentioned this pull request Apr 2, 2025
Closed
@skirpichev skirpichev self-requested a review April 3, 2025 01:14
skirpichev
skirpichev previously approved these changes Apr 3, 2025
View reviewed changes
@Prometheus3375
Copy link
Contributor Author

Prometheus3375 commented Apr 3, 2025 •
edited
Loading

  1. I didn't like the use of "thousands separator", as I believe for fractional part "thousandths" is more correct and it made sense only for decimal representation. Instead, I replaced "thousands separator" with "digit separator".
  2. I did as well with the description of type n which used "number separator". For floating-point type n, I also noted that it specifies separators only for integral part.
  3. Added an example of underscore separator with octal type to showcase digit groups of 4.

@skirpichev skirpichev self-requested a review April 3, 2025 11:55
skirpichev
skirpichev approved these changes Apr 4, 2025
View reviewed changes
Copy link
Member

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

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

I didn't like the use of "thousands separator", as I believe for fractional part "thousandths" is more correct and it made sense only for decimal representation. Instead, I replaced "thousands separator" with "digit separator".

That was, probably, an artifact of PEP 378. I agree, "thousands separator" looks rather odd for base-2 integer representation types.

CC @picnixz

@picnixz picnixz self-requested a review April 4, 2025 12:58
picnixz
picnixz reviewed Apr 4, 2025
View reviewed changes
Prometheus3375 and others added 3 commits April 4, 2025 22:03
@Prometheus3375 @picnixz
Add line breaks after versionchanged
dd9acb6 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@Prometheus3375
Add missing the
0c8a036
@Prometheus3375
Use digit group separator
c6a175f
picnixz
picnixz reviewed Apr 4, 2025
View reviewed changes
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Some final wording comments

@Prometheus3375 @picnixz
Add missing the
fd55c51 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@picnixz picnixz self-assigned this Apr 4, 2025
@picnixz
Copy link
Member

picnixz commented Apr 4, 2025

I'll merge this tomorrow! (I don't merge stuff after a certain hour)

@Prometheus3375
Copy link
Contributor Author

For 3.13 and below it would be better to prepare separate PR as grouping for fractional part is added in 3.14. I can make one after this is merged.

@picnixz
Copy link
Member

picnixz commented Apr 4, 2025 •
edited
Loading

If you can do your own PR for 3.13 and below, please do so! Just @ me afterwards

@skirpichev
Copy link
Member

For 3.13 and below it would be better to prepare separate PR as grouping for fractional part is added in 3.14. I can make one after this is merged.

Yes, I think it worth backporting to 3.12 and 3.13. It should be mostly trivial (minus extra paragraph), modulo changes in the grammar.

@picnixz picnixz merged commit 06a110f into python:main Apr 5, 2025
28 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in Docs PRs Apr 5, 2025
@picnixz
Copy link
Member

picnixz commented Apr 5, 2025

Thank you for the fix! you can now do the backports for 3.13 only (I'll make an automatic backport for 3.12 as I don't think it didn't change between 3.12 and 3.13)

@bedevere-app
Copy link

bedevere-app bot commented Apr 5, 2025

GH-132123 is a backport of this pull request to the 3.13 branch.

@Prometheus3375 Prometheus3375 deleted the gh-131912 branch April 5, 2025 13:01
@bedevere-app
Copy link

bedevere-app bot commented Apr 5, 2025

GH-132123 is a backport of this pull request to the 3.13 branch.

@Prometheus3375 Prometheus3375 mentioned this pull request Apr 6, 2025
Merged
picnixz pushed a commit that referenced this pull request Apr 7, 2025
@Prometheus3375
[3.13] gh-131912: Improve description of grouping options in the form…
07483c2 
…at specification docs (GH-132030) (#132123)

(cherry picked from commit 06a110f)
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 7, 2025
@Prometheus3375 @miss-islington
[3.13] pythongh-131912: Improve description of grouping options in th…
be7c13a 
…e format specification docs (pythonGH-132030) (pythonGH-132123)

(cherry picked from commit 07483c2)

Co-authored-by: Prometheus3375 <35541026+Prometheus3375@users.noreply.github.com>
(cherry picked from commit 06a110f)
@bedevere-app
Copy link

bedevere-app bot commented Apr 7, 2025

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

picnixz pushed a commit that referenced this pull request Apr 7, 2025
@miss-islington @Prometheus3375
[3.12] gh-131912: Improve description of grouping options in the form…
ab78a37 
…at specification docs (GH-132030) (#132203)

(cherry picked from commit 06a110f)

Co-authored-by: Prometheus3375 <35541026+Prometheus3375@users.noreply.github.com>
seehwan pushed a commit to seehwan/cpython that referenced this pull request Apr 16, 2025
@Prometheus3375 @picnixz @seehwan80
pythongh-131912: Improve description of grouping options in the forma…
32ab983 
…t specification docs (python#132030)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skirpichev skirpichev skirpichev approved these changes

@picnixz picnixz picnixz approved these changes

Assignees

@picnixz picnixz

Labels
docs Documentation in the Doc dir skip news
Projects
Docs PRs
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Prometheus3375 @picnixz @skirpichev