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

bpo-40017: add CLOCK_TAI constant to the time module, if the constant is defined #19096

Merged
merged 11 commits into from
Mar 24, 2020
Merged

bpo-40017: add CLOCK_TAI constant to the time module, if the constant is defined #19096

merged 11 commits into from
Mar 24, 2020

Conversation

r-owen
Copy link
Contributor

@r-owen r-owen commented Mar 21, 2020 •
edited
Loading

Added the CLOCK_TAI constant to the time module, if present (and checked that it worked on ubuntu).

Added a description to the time module documentation.

I will gladly add a unit test if somebody can point me to the existing tests for these constants (a global search for CLOCK_BOOTTIME didn't turn up anything.

Would it make any sense to add this to the next Python 8.x as well?

https://bugs.python.org/issue40017

@r-owen r-owen requested review from abalkin and pganssle as code owners March 21, 2020 00:34
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@r-owen

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@r-owen
Copy link
Contributor Author

r-owen commented Mar 21, 2020 •
edited
Loading

My Python tracker username is r3owen. My github username is r_owen and I already set that in the details for my bug tracker account and signed the contribution agreement.

@r-owen r-owen changed the title bpo-40117: add CLOCK_TAI constant to the time module, if the constant is defined bpo-40017: add CLOCK_TAI constant to the time module, if the constant is defined Mar 21, 2020
benjaminp
benjaminp requested changes Mar 21, 2020
View reviewed changes
Doc/library/time.rst Outdated
@@ -774,6 +774,16 @@ These constants are used as parameters for :func:`clock_getres` and

.. versionadded:: 3.7

.. data:: CLOCK_TAI

International atomic time, as seconds from the unix epoch.
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems erroneous to say "seconds" here, since the constant merely implies a a clock not a unit. It might be helpful to have a sentence about how International Atomic Time—which should be capitalized by the way—is different from UTC. Or at least provide a link to an official page about it if there is one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add a link though I worry that the long line will be unwelcome.

Note that computer clock time is not actually UTC (my text in the issue was not fully correct), though it is closely related. This makes it difficult to explain in a few words what this does. But anyone who wants TAI will be pleased to have CLOCK_TAI.

There is also a CLOCK_UTC constant I could expose, but I do not see how it can be used to return anything useful. For proper UTC handling at a leap second it is necessary to express the date as a datetime object, ISO format, or similar; a duration since the epoch is not good enough.

@bedevere-bot
Copy link

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.

@r-owen
Copy link
Contributor Author

r-owen commented Mar 21, 2020

I made the requested changes. If the long text in time.rst is a problem I would appreciate a suggestion how to resolve it.

@r-owen
Copy link
Contributor Author

r-owen commented Mar 21, 2020

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@benjaminp: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from benjaminp March 21, 2020 15:08
vstinner
vstinner reviewed Mar 21, 2020
View reviewed changes
Doc/library/time.rst
@@ -805,7 +815,6 @@ These constants are used as parameters for :func:`clock_getres` and

.. versionadded:: 3.8


Copy link
Member

Choose a reason for hiding this comment

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

I like having two empty lines, it helps readibility. Please keep it ;-)

Misc/NEWS.d/next/Library/2020-03-21-00-46-18.bpo-40017.HFpHZS.rst Outdated
@@ -0,0 +1 @@
Expose the CLOCK_TAI constant to the time library if the operating system support it.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Expose the CLOCK_TAI constant to the time library if the operating system support it.
Add :data:`time.CLOCK_TAI` constant, if the operating system support it: International Atomic Time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You and @benjaminp seem to have different ideas for this entry. I've gone with @vstinner's version for now, but @benjaminp if you want something else perhaps the two of you can agree?

@r-owen
Copy link
Contributor Author

r-owen commented Mar 21, 2020

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@benjaminp: please review the changes made to this pull request.

vstinner
vstinner approved these changes Mar 21, 2020
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I let @benjaminp merge it :-)

@benjaminp benjaminp force-pushed the 40017_add_clock_tai branch from 8b7d6c7 to 903741c Compare March 24, 2020 03:17
@benjaminp benjaminp force-pushed the 40017_add_clock_tai branch from 903741c to 22db5e6 Compare March 24, 2020 03:20
@benjaminp benjaminp merged commit 6000087 into python:master Mar 24, 2020
@timj timj mentioned this pull request Mar 25, 2020
Closed
@r-owen r-owen deleted the 40017_add_clock_tai branch April 7, 2020 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@abalkin abalkin Awaiting requested review from abalkin

@pganssle pganssle Awaiting requested review from pganssle

@benjaminp benjaminp Awaiting requested review from benjaminp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@r-owen @the-knights-who-say-ni @bedevere-bot @vstinner @benjaminp