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.

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

gh-108202: calendar: Document prweek #108466

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

gh-108202: calendar: Document prweek #108466

wants to merge 4 commits into from
+5 −0

Conversation

apurvakhatri
Copy link

@apurvakhatri apurvakhatri commented Aug 25, 2023 •
edited by bedevere-bot
Loading

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Aug 25, 2023 •
edited
Loading

The following commit authors need to sign the Contributor License Agreement:

Click the button to sign:
CLA not signed

@bedevere-bot bedevere-bot added docs Documentation in the Doc dir skip news awaiting review labels Aug 25, 2023
@Eclips4

This comment was marked as outdated.

Sign in to view
@Eclips4 Eclips4 closed this Aug 25, 2023
@AA-Turner

This comment was marked as outdated.

Sign in to view
@AA-Turner AA-Turner reopened this Aug 25, 2023
@AA-Turner AA-Turner changed the title Fix issue 108202 gh-108202: Document prweek and mdays Aug 25, 2023
@bedevere-bot bedevere-bot mentioned this pull request Aug 25, 2023
Open
@AlexWaygood AlexWaygood changed the title gh-108202: Document prweek and mdays gh-108202: calendar: Document prweek and mdays Aug 25, 2023
@AA-Turner
Copy link
Member

@apurvakhatri please could you split this PR into two parts? We haven't discussed if we want to document mdays, so having both in one PR makes it difficult to review.

A

@apurvakhatri
Copy link
Author

Sure, will do that! Should I create a new PR or just remove mdays from this PR request?

@AA-Turner
Copy link
Member

The latter is likely easier. Perhaps also consider documenting all prweek() instances in this PR, rather than just TextCalendar.prweek().

A

@Eclips4

This comment was marked as outdated.

Sign in to view
@AA-Turner
Copy link
Member

@apurvakhatri please avoid force pushes, see the devguide:

Your pull request may involve several commits as a result of addressing code review comments. Please keep the commit history in the pull request intact by not squashing, amending, or anything that would require a force push to GitHub. A detailed commit history allows reviewers to view the diff of one commit to another so they can easily verify whether their comments have been addressed. The commits will be squashed when the pull request is merged.

@AA-Turner
Copy link
Member

Please also restrict this PR to just prweek(), as discussed.

@AA-Turner AA-Turner changed the title gh-108202: calendar: Document prweek and mdays gh-108202: calendar: Document prweek Sep 6, 2023
@AA-Turner AA-Turner changed the title gh-108202: calendar: Document prweek gh-108202: calendar: Document prweek Sep 6, 2023
hugovk
hugovk requested changes Aug 15, 2024
View reviewed changes
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

@apurvakhatri Hello! Please could you sign the CLA? We can't merge before then.

Doc/library/calendar.rst
@@ -138,6 +138,11 @@ interpreted as prescribed by the ISO 8601 standard. Year 0 is 1 BC, year -1 is

:class:`TextCalendar` instances have the following methods:

.. method:: prweek(theweek, width)

Print a week's calendar as returned by :meth:`formatweek` and sets the
Copy link
Member

Choose a reason for hiding this comment

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

This will silence the warning (it can be reverted when the formatweek documentation is added):

Suggested change
Print a week's calendar as returned by :meth:`formatweek` and sets the
Print a week's calendar as returned by :meth:`!formatweek` and sets the

@bedevere-app
Copy link

bedevere-app bot commented Aug 15, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@hugovk hugovk added the pending The issue will be closed if no feedback is provided label Sep 5, 2024
@StanFromIreland
Copy link
Contributor

@apurvakhatri you may have forgotten about this PR however if you would like to see it merged please look into signing the CLA.

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

@hugovk hugovk hugovk requested changes

Assignees
No one assigned
Labels
awaiting changes docs Documentation in the Doc dir pending The issue will be closed if no feedback is provided skip news
Projects
Docs PRs
Status: Todo
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@apurvakhatri @Eclips4 @AA-Turner @StanFromIreland @hugovk @bedevere-bot