Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-85453: Fix missing/wrong backquotes and role texts in datetime documentation#21447
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
yyyyyyyan commented Jul 12, 2020 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
9a4d299 to 325824bCompare
nanjekyejoannah left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am curious, why the "." before every item i.e classes, fields etc
yyyyyyyan commented Jul 20, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@nanjekyejoannah this is mainly to prevent future ambiguity bugs, but also fixes some current bugs we have now. A quick example is on line 1214 of datetime.rst in master, where we have which ends up linking to the module |
serverwentdown left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed this, I think this is a significant improvement, but also I left some considerations.
| * A millisecond is converted to ``1000`` microseconds. | ||
| * A minute is converted to ``60`` seconds. | ||
| * An hour is converted to ``3600`` seconds. | ||
| * A week is converted to ``7`` days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'd disagree with these changes as these numbers aren't code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I take that back, this is fine. I perceive them as referring to microseconds and not microseconds, seconds and not seconds, etc.
Maybe the arguments in these four lines could be made into italics too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All:
- A millisecond is converted to
1000microseconds. - A minute is converted to
60seconds. - An hour is converted to
3600seconds. - A week is converted to
7days.
Or none:
- A millisecond is converted to 1000 microseconds.
- A minute is converted to 60 seconds.
- An hour is converted to 3600 seconds.
- A week is converted to 7 days.
Uh oh!
There was an error while loading. Please reload this page.
| It's common for this to be restricted to years from 1970 through 2038. Note | ||
| that on non-POSIX systems that include leap seconds in their notion of a | ||
| timestamp, leap seconds are ignored by :meth:`fromtimestamp`. | ||
| timestamp, leap seconds are ignored by :meth:`.date.fromtimestamp`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think the dots are excessive, because it is very unlikely that a global module named fromtimestamp is going to exist in the future.
serverwentdownFeb 11, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the likelihood of ambiguity in the future is quite small, therefore the significant number of changes isn't really justified. For me, I find the docs source code harder to read with all the "."s.
Co-authored-by: Ambrose Chua <[email protected]>
slateny commented Dec 10, 2022
Regarding not splitting up the PR, to the contrary, I would actually recommend at least another PR just for the roles (but perhaps more depending on what remains). At its current state, it's difficult to review due to its length combined with the formatting changes interlaced between the role text changes. |
erlend-aasland commented Feb 9, 2024
Please split this up in smaller parts. Try to keep the diff within ~200 lines of code for each PR. |
ghost commented Mar 19, 2024
The following commit authors need to sign the Contributor License Agreement: |
nanjekyejoannah commented Mar 19, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Why should we split a PR on a single topic especially a docs PR @erlend-aasland ? Is this rule new since I haven't been following core dev stuff due to my schedule these days. |
nanjekyejoannah commented Mar 19, 2024
I reopened in error, sorry |
erlend-aasland commented Mar 19, 2024
The PR does more than the title says:
We advocate making atomic code and documentation changes. A PR that consists of many different types of changes is hard to review and has a high risk of becoming a stale PR.
I guess so. The last one or two years, we've chosen to adapt Diátaxis as our north star for documentation. Among other things, it advocates to carry out smaller changes in iterations. Moreover, the diff is so large that even GitHub does not show it by default. Back to the PR: some of these changes are trivial, and would be easy to review and merge as separate PRs; other changes are non-trivial, and require more discussion IMO; lastly, another part of the changes are IMO style changes that are perhaps not needed. |
Also fix some small typos. I know it's a lot of change to review but I really don't think it would be worth to separate these commits in different PRs.
https://bugs.python.org/issue41281