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

Jump to bottom

gh-85432: Harmonise parameter names between C and pure-Python implementations of datetime.time.strftime, datetime.datetime.fromtimestamp #99993

Merged
merged 1 commit into from Dec 22, 2022
Merged

gh-85432: Harmonise parameter names between C and pure-Python implementations of datetime.time.strftime, datetime.datetime.fromtimestamp #99993

merged 1 commit into from Dec 22, 2022
+18 −5

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Dec 4, 2022

#85432 was closed slightly prematurely in my view. While #21712 fixes things for datetime.date.strftime, as mentioned in #85432 (comment), the problem also exists for datetime.time.strftime, as well as datetime.datetime.fromtimestamp:

Works with the C implementation:

>>> import time, datetime
>>> datetime.time().strftime(format="%f")
'000000'
>>> datetime.datetime.fromtimestamp(timestamp=time.time())
datetime.datetime(2022, 12, 4, 16, 23, 25, 69430)
>>> exit()

Not with the pure-Python implementation:

>>> import sys
>>> sys.modules["_datetime"] = None
>>> import time, datetime
>>> datetime.time().strftime(format="%f")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: time.strftime() got an unexpected keyword argument 'format'
>>> datetime.datetime.fromtimestamp(timestamp=time.time())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: datetime.fromtimestamp() got an unexpected keyword argument 'timestamp'

@AlexWaygood
pythongh-85432: harmonise parameter names between C and pure-Python i…
bcb065d 
…mplementations of `datetime.time.strftime`, `datetime.datetime.fromtimestamp`
@AlexWaygood AlexWaygood requested a review from iritkatriel Dec 4, 2022
@bedevere-bot bedevere-bot mentioned this pull request Dec 4, 2022
Closed
@hugovk
Copy link
Member

hugovk commented Dec 4, 2022

One issue with renaming fmt to format is it shadows the built-in:

image

Not specifically a problem right now in these functions, but it may encourage users to also name a variable format in their calling code (I often mirror my own variables names <-> parameter names) and cause frustrating errors later.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Dec 4, 2022 •
edited

One issue with renaming fmt to format is it shadows the built-in:

I agree, but the same change was just made to datetime.date.strftime in #21712, and I think it's more important to keep things internally consistent within the datetime module. Also: the parameter is already named format in the C implementation, which is the implementation more commonly used by end users. Changing the parameter name in the C implementation would be a much more severe breaking change than what I'm proposing :)

hauntsaninja
hauntsaninja approved these changes Dec 22, 2022
View changes
Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

This looks good to me! I agree that the parameter should be renamed format to avoid making a breaking change in the C implementation.

cc @JelleZijlstra

@hauntsaninja hauntsaninja merged commit 9cdb642 into python:main Dec 22, 2022
15 checks passed
@AlexWaygood AlexWaygood deleted the fromtimestamp branch Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra

@hauntsaninja hauntsaninja

@iritkatriel iritkatriel

@pganssle pganssle

@abalkin abalkin

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@AlexWaygood @hugovk @JelleZijlstra @hauntsaninja @bedevere-bot