Skip to content

gh-108202: calendar: Document TextCalendar methods #127608

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 14 commits into from
Jan 3, 2025
Merged

gh-108202: calendar: Document TextCalendar methods #127608

merged 14 commits into from
Jan 3, 2025

Conversation

StanFromIreland
Copy link
Contributor

@StanFromIreland StanFromIreland commented Dec 4, 2024 •
edited by github-actions bot
Loading

@bedevere-app bedevere-app bot added docs Documentation in the Doc dir skip news labels Dec 4, 2024
@bedevere-app bedevere-app bot mentioned this pull request Dec 4, 2024
Open
@StanFromIreland StanFromIreland marked this pull request as ready for review December 5, 2024 16:27
@StanFromIreland

This comment was marked as outdated.

Sign in to view
ZeroIntensity
ZeroIntensity reviewed Dec 10, 2024
View reviewed changes
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Some initial comments. (I'll tag Erlend again once I consider this ready from a triage standpoint.)

It's probably worth converting the phrase "string representation" into a reference to the str class.

@ZeroIntensity ZeroIntensity added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Dec 10, 2024
StanFromIreland and others added 5 commits December 10, 2024 16:04
@StanFromIreland @ZeroIntensity
Update Doc/library/calendar.rst
e3a14b0 
Co-authored-by: Peter Bierma <[email protected]>
@StanFromIreland @ZeroIntensity
Update Doc/library/calendar.rst
a41d652 
Co-authored-by: Peter Bierma <[email protected]>
@StanFromIreland @ZeroIntensity
Update Doc/library/calendar.rst
238befb 
Co-authored-by: Peter Bierma <[email protected]>
@StanFromIreland @ZeroIntensity
Update Doc/library/calendar.rst
781876c 
Co-authored-by: Peter Bierma <[email protected]>
@StanFromIreland
Change string to :class:str
60b524f 
As per ZeroIntenisities suggestion
@Antony3293 Antony3293 mentioned this pull request Dec 10, 2024
Closed
ZeroIntensity
ZeroIntensity reviewed Dec 11, 2024
View reviewed changes
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Ok, I'll refrain from nitpicking. I don't know enough about calendar to give this a green check, so I'll just tag Erlend.

eendebakpt
eendebakpt reviewed Dec 17, 2024
View reviewed changes
eendebakpt
eendebakpt approved these changes Dec 29, 2024
View reviewed changes
@StanFromIreland
Copy link
Contributor Author

@erlend-aasland All done now. In the future do you prefer a PR for every function like hugokvs maybe it is easier to review?

@erlend-aasland
Copy link
Contributor

No, do not create one PR for every function. Just don't mix different types of changes in a PR (nor in an issue). This is all explained in clear text in the devguide.

@StanFromIreland
Copy link
Contributor Author

@erlend-aasland ok. Is this PR ok for you then? Are there any other changes you want before you review? This PR was originally focused on just the functions but then expanded with suggestions from other reviewers, I have reverted all of them.

@erlend-aasland
Copy link
Contributor

Is this PR ok for you then?

I'm torn about these changes. If you take the time to read through this issue's history, you'll find that it is in fact pretty controversial. Some core devs actively oppose documenting methods that could/should be considered helpers and/or private APIs. Other core devs lean towards that every (intentional or unintentional) public API should be documented. There is no happy path.

I guess I'm fine with this; since other similar calendar formatting helpers have been documented, we might as well document the rest of them.

@erlend-aasland
Copy link
Contributor

This PR was originally focused on just the functions but then expanded with suggestions from other reviewers, I have reverted all of them.

I see; I'm sorry about the extra work. I did not read through all of the previous remarks. I prefer to align strictly with our recommended development guidelines and strictly keep any type of change atomic. This implies:

  • not mixing multiple bugfixes
  • not mixing bugfixes and features
  • not mixing multiple features (related or unrelated)
  • not mixing different types of documentation enhancements or "bugfixes"

As with any kind of rule, there will be exceptions. But exceptions are exceptions; if they become the new norm, our guidelines become moot.

Keeping changes atomic has several positive effects:

  • changes are easier to review
  • code changes are easier to debug1
  • backporting is made easier
  • reverting problematic changes is easier

Footnotes

  1. note that git bisect becomes pointless if you mix bugfixes and/or features in a single PR ↩

@erlend-aasland
Copy link
Contributor

@picnixz: if you are satisfied with this, feel free to land it.

picnixz
picnixz reviewed Jan 3, 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.

A last markup change and we're good.

@StanFromIreland @picnixz
Update Doc/library/calendar.rst
bfbfaf6 
Co-authored-by: Bénédikt Tran <[email protected]>
@picnixz
Copy link
Member

picnixz commented Jan 3, 2025

(FTR, I still don't have the commit rights so you'll still need to hit the merge button @erlend-aasland :'))

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jan 3, 2025 •
edited
Loading

(FTR, I still don't have the commit rights [...]

What?! Outrageous! 😄

@erlend-aasland erlend-aasland merged commit bb73426 into python:main Jan 3, 2025
25 checks passed
@miss-islington-app

This comment was marked as outdated.

Sign in to view
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 3, 2025
@StanFromIreland @miss-islington
pythongh-108202: Document calendar.TextCalendar formatting helpers (p…
96bc8d5 
…ythonGH-127608)

Document the following TextCalendar methods:
- formatday()
- formatmonthname()
- formatweekday()
- formatweekheader()
(cherry picked from commit bb73426)

Co-authored-by: Stan Ulbrych <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 3, 2025
@StanFromIreland @miss-islington
pythongh-108202: Document calendar.TextCalendar formatting helpers (p…
21f12ef 
…ythonGH-127608)

Document the following TextCalendar methods:
- formatday()
- formatmonthname()
- formatweekday()
- formatweekheader()
(cherry picked from commit bb73426)

Co-authored-by: Stan Ulbrych <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jan 3, 2025

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

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jan 3, 2025
@bedevere-app
Copy link

bedevere-app bot commented Jan 3, 2025

GH-128441 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 3, 2025
erlend-aasland pushed a commit that referenced this pull request Jan 3, 2025
@miss-islington @StanFromIreland
[3.12] gh-108202: Document calendar.TextCalendar formatting helpers (G…
0d3f4b6 
…H-127608) (#128441)

Document the following TextCalendar methods:
- formatday()
- formatmonthname()
- formatweekday()
- formatweekheader()

(cherry picked from commit bb73426)

Co-authored-by: Stan Ulbrych <[email protected]>
erlend-aasland pushed a commit that referenced this pull request Jan 3, 2025
@miss-islington @StanFromIreland
[3.13] gh-108202: Document calendar.TextCalendar formatting helpers (G…
e0f0d2d 
…H-127608) (#128440)

Document the following TextCalendar methods:
- formatday()
- formatmonthname()
- formatweekday()
- formatweekheader()

(cherry picked from commit bb73426)

Co-authored-by: Stan Ulbrych <[email protected]>
WolframAlph pushed a commit to WolframAlph/cpython that referenced this pull request Jan 4, 2025
@StanFromIreland @WolframAlph
pythongh-108202: Document calendar.TextCalendar formatting helpers (p…
21b4a29 
…ython#127608)

Document the following TextCalendar methods:
- formatday()
- formatmonthname()
- formatweekday()
- formatweekheader()
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
@StanFromIreland @srinivasreddy
pythongh-108202: Document calendar.TextCalendar formatting helpers (p…
f529d7f 
…ython#127608)

Document the following TextCalendar methods:
- formatday()
- formatmonthname()
- formatweekday()
- formatweekheader()
@StanFromIreland StanFromIreland deleted the patch-4 branch February 11, 2025 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eendebakpt eendebakpt eendebakpt approved these changes

@picnixz picnixz picnixz approved these changes

@erlend-aasland erlend-aasland erlend-aasland approved these changes

@ZeroIntensity ZeroIntensity Awaiting requested review from ZeroIntensity

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.
6 participants
@StanFromIreland @eendebakpt @ZeroIntensity @hugovk @erlend-aasland @picnixz