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.

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

bpo-41281: Fix missing/wrong backquotes and role texts in datetime documentation #21447

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

Conversation

yyyyyyyan
Copy link
Contributor

@yyyyyyyan yyyyyyyan commented Jul 12, 2020

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

Copy link
Member

@nanjekyejoannah nanjekyejoannah left a comment

Am curious, why the "." before every item i.e classes, fields etc

@yyyyyyyan
Copy link
Contributor Author

yyyyyyyan commented Jul 20, 2020

@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

See also method :meth:`time` 

which ends up linking to the module time. Prefixing the dot specifies we're in datetime.datetime class scope and therefore the only possible link is to the datetime.datetime.time() method.

@yyyyyyyan yyyyyyyan requested a review from nanjekyejoannah Oct 2, 2020
Copy link
Contributor

@serverwentdown serverwentdown left a comment

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.
Copy link
Contributor

@serverwentdown serverwentdown Feb 11, 2021

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.

Copy link
Contributor

@serverwentdown serverwentdown Feb 11, 2021

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.

Copy link
Contributor

@serverwentdown serverwentdown Feb 11, 2021

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 1000 microseconds.
  • A minute is converted to 60 seconds.
  • An hour is converted to 3600 seconds.
  • A week is converted to 7 days.

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.

Doc/library/datetime.rst Outdated Show resolved Hide resolved
of the range of values supported by the platform C :c:func:`localtime`
function, and :exc:`OSError` on :c:func:`localtime` failure.
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`.
Copy link
Contributor

@serverwentdown serverwentdown Feb 11, 2021

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.

Copy link
Contributor

@serverwentdown serverwentdown Feb 11, 2021

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 <ambrose@hey.com>
@slateny
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review docs Documentation in the Doc dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants