10000 Trivial Change: Remove unhelpful doc in `datetime.timedelta` by meawoppl · Pull Request #100164 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

Trivial Change: Remove unhelpful doc in datetime.timedelta #100164

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 2 commits into from
Feb 6, 2023
Merged

Trivial Change: Remove unhelpful doc in datetime.timedelta #100164

merged 2 commits into from
Feb 6, 2023

Conversation

meawoppl
Copy link
Contributor

I use timedelta's often in my work, and it always makes me a little sad to see this when I mouse over to look at the docstring.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@netlify
Copy link
netlify bot commented Dec 11, 2022 •
edited
Loading

✅ Deploy Preview for python-cpython-preview ready!

Name Link
🔨 Latest commit 5b35c1a
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/63953d608a84c80009db6715
😎 Deploy Preview https://deploy-preview-100164--python-cpython-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ghost
Copy link
ghost commented Dec 11, 2022 •
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

abalkin
abalkin approved these changes Dec 11, 2022
View reviewed changes
Copy link
Member
@abalkin abalkin left a comment

Choose a reason for hiding this comment

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

LGTM

@abalkin
Copy link
Member
abalkin commented Dec 11, 2022

@meawoppl - Thank you for your contribution! I don't think small changes like this should require a signed CLA, but it is probably easier to sign it with two clicks than to figure out how to make an exception.

Please follow the instructions above at #100164 (comment).

@pganssle
Copy link
Member
pganssle commented Jan 5, 2023

I use timedelta's often in my work, and it always makes me a little sad to see this when I mouse over to look at the docstring.

I don't really understand why you are seeing this at all. The docstring for timedelta should be coming from _datetimemodule.c, and it's this:

Difference between two datetime values.

timedelta(days=0, seconds=0, microseconds=0, milliseconds=0, minutes=0, hours=0, weeks=0)

All arguments are optional and default to 0.
Arguments may be integers or floats, and may be positive or negative.

I am not super attached to the "Why? Because I felt like it" wording, but it is also kind of useful information to know that this was an arbitrary choice and not one that was the result of a great amount of deliberation. It's a guard against a future where changes to the structure of timedelta are held up by Chesterton's Fence. If we can find an appropriate place to put a comment of this sort, that would be best.

@meawoppl
Copy link
Contributor Author

@pganssle I noticed it because some of the VSCode hover-for-help features show it as part of the docstring. Not sure why it isn't being masked....

The signature process produces a 500 error:
image
...but seems to have worked anyway?

@meawoppl
Copy link
Contributor Author

@abalkin I signed the agreement. Also, I suspect this is below the "news entry" level of diff, but happy to update if not.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@abalkin
Copy link
Member
abalkin commented Jan 29, 2023 •
edited
Loading

it is also kind of useful information to know that this was an arbitrary choice and not one that was the result of a great amount of deliberation.

I will leave it to @pganssle to make a final call on this. Even though git attributes the authorship to me, I suspect that it was @tim-one who wrote that docstring. We can compromise on moving the "unhelpful doc" to a # comment below the docstring.

@meawoppl
Copy link
Contributor Author

Happy to change it up that way, just let me know if that is what is desired.

CuriousLearner
CuriousLearner approved these changes Feb 5, 2023
View reviewed changes
Copy link
Member
@CuriousLearner CuriousLearner 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 agree with @abalkin that the comment on rationale of having that representation can be moved to a separate comment below docstring, and maybe we can also improve it saying the original author found that representation useful/helpful.

@pganssle
Copy link
Member
pganssle commented Feb 5, 2023

I've gone ahead and added a comment below the docstring and pushed directly to @meawoppl 's branch. I also did a rebase against main.

I think we can go ahead and merge this, but I'll give it 24 hours for @meawoppl and/or @abalkin to weigh in on the new wording if desired.

CuriousLearner
CuriousLearner approved these changes Feb 5, 2023
View reviewed changes
Copy link
Member
@CuriousLearner CuriousLearner left a comment

Choose a reason for hiding this comment

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

LGTM!

@abalkin abalkin merged commit d3e2dd6 into python:main Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abalkin abalkin abalkin approved these changes

@CuriousLearner CuriousLearner CuriousLearner approved these changes

@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner

Assignees
No one assigned
Labels
skip issue skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@meawoppl @bedevere-bot @abalkin @pganssle @CuriousLearner
0