Fix datetime.strftime #6317

karolyi · 2021-11-16T16:50:04Z

datetime.strftime() takes format: str as a positional argument.

`datetime.strftime()` takes `format: str` as a positional argument.

karolyi · 2021-11-16T16:51:24Z

In [1]: from datetime import datetime

In [2]: a = datetime.now()

In [3]: a.strftime(fmt='%c')
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-b6d3aae8d1f0> in <module>
----> 1 a.strftime(fmt='%c')

TypeError: strftime() missing required argument 'format' (pos 1)

In [4]: a.strftime(format='%c')
Out[4]: 'Tue Nov 16 17:50:47 2021'

AlexWaygood · 2021-11-16T17:03:31Z

Same goes for datetime.time.strftime and datetime.date.strftime

srittau · 2021-11-16T17:44:53Z

Unfortunately, it's not that easy. The pure-Python version of datetime et al. names this argument fmt. It's probably safest to make this a positional-only argument if the two available runtime versions of this method don't match.

karolyi · 2021-11-16T18:04:21Z

True. I dived into the source to see where, if any, a C module gets imported. It's towards the end. That implementation is the one that uses format, and the one in the pure python version uses fmt.

Maybe typeshed could check for datetime.__doc__ (see the last line in datetime.py) to see what version of the module is imported.

Akuli · 2021-11-16T18:09:17Z

To me, a positional only argument makes sense the most. I would want to get errors for both strftime(format="lol") and strftime(fmt="lol"), as neither of them will work correctly in all cases.

AlexWaygood · 2021-11-16T18:10:13Z

Maybe typeshed could check for datetime.__doc__ (see the last line in datetime.py) to see what version of the module is imported.

Personally, I think it's a bit of a bug that the parameter has a different name in the C implementation and the Python implementation; I'd be inclined to report it as a bug on bugs.python.org and try to get that resolved.

srittau · 2021-11-16T18:10:59Z

To me, a positional only argument makes sense the most. I would want to get errors for both strftime(format="lol") and strftime(fmt="lol"), as neither of them will work correctly in all cases.

Although strftime(format="lol") could easily be reduced to "lol". 😈 (Sorry, I'm tired ...)

karolyi · 2021-11-16T18:12:10Z

Maybe typeshed could check for datetime.__doc__ (see the last line in datetime.py) to see what version of the module is imported.

Personally, I think it's a bit of a bug that the parameter has a different name in the C implementation and the Python implementation; I'd be inclined to report it as a bug on bugs.python.org and try to get that resolved.

I'd be with you on that, but generally, bugfixes don't happen very fast.

(I like to pass positional arguments, makes the code more readable)

AlexWaygood · 2021-11-16T18:26:16Z

Seems like there already is an open BPO issue about this here

karolyi · 2021-11-16T18:36:56Z

As mentioned in the other issue, I'm for changing it to format: str.

srittau · 2021-11-16T18:40:49Z

While I would prefer we change this to a positional only for now, I'm fine with changing it to format per the linked bpo issue.

karolyi · 2021-11-16T18:44:27Z

I can make the correction for date.strftime() and time.strftime() as well, but there seems to be a conflict now which I don't really understand.

It looks as though a couple lines disappeared from the master branch?

AlexWaygood · 2021-11-16T18:45:48Z

I can make the correction for date.strftime() and time.strftime() as well, but there seems to be a conflict now which I don't really understand.

It looks as though a couple lines disappeared from the master branch?

Yes, I, er, just deleted datetime.datetime.strftime, as it's inherited from datetime.date at runtime, so doesn't need to be redefined. It was this PR: #6320. The only functions you should need to alter are date.strftime and time.strftime.

srittau

LGTM, but I'd like to hear @Akuli's opinion.

karolyi · 2021-11-16T19:01:19Z

thereifixedit™

hauntsaninja · 2021-11-16T19:06:46Z

I agree with Akuli that this should be positional-only till the fix for https://bugs.python.org/issue41260 is merged, at which point we can branch on sys.version (and yes, I agree that format is what CPython should prefer).

Akuli · 2021-11-17T23:21:39Z

I still think a positional-only parameter would be better for now, but I'm fine with it either way.

JelleZijlstra · 2021-11-18T05:54:55Z

stdlib/datetime.pyi

@@ -55,7 +55,7 @@ class date:
    @property
    def day(self) -> int: ...
    def ctime(self) -> str: ...
-    def strftime(self, fmt: str) -> str: ...
+    def strftime(self, format: str) -> str: ...


Suggested change

def strftime(self, format: str) -> str: ...

def strftime(self, __format: str) -> str: ...

Let's make it positional-only as discussed.

Quoting https://bugs.python.org/msg373421, specifically:

I think in typeshed they can safely change from fmt to format even today (which would almost certainly be more accurate to end user use cases).

What do you have against this?

Type checking is all about warning users about situations where their code could lead to unexpected errors. Passing a keyword argument to this parameter will currently cause code to raise an error sometimes (but not all the time, or even most of the time), as not all systems can be guaranteed to use the C implementation of datetime (this is partly why the Python implementation exists alongside the C implementation). If a user passes a positional argument to this parameter, however, then there is no possibility of an error. So, it seems correct that, as the situation currently stands, a type-checker should raise an error if a keyword argument is passed to this parameter, as doing so could in some situations lead to unexpected errors.

It's definitely better than the current situation to have the parameter named format, but I agree that the best option for now is to have it as a positional-only parameter in the stub.

JelleZijlstra · 2021-11-18T05:55:03Z

stdlib/datetime.pyi

@@ -118,7 +118,7 @@ class time:
    if sys.version_info >= (3, 7):
        @classmethod
        def fromisoformat(cls: Type[_S], time_string: str) -> _S: ...
-    def strftime(self, fmt: str) -> str: ...
+    def strftime(self, format: str) -> str: ...


Suggested change

def strftime(self, format: str) -> str: ...

def strftime(self, __format: str) -> str: ...

github-actions · 2021-12-01T06:53:50Z

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Fix datetime.strftime

c8ace72

`datetime.strftime()` takes `format: str` as a positional argument.

This comment has been minimized.

Sign in to view

srittau closed this Nov 16, 2021

srittau reopened this Nov 16, 2021

This comment has been minimized.

Sign in to view

karolyi added 2 commits November 16, 2021 18:47

Merge branch 'master' into patch-1

0c52db0

strftime(fmt: str) -> strftime(format: str)

c855b8f

srittau approved these changes Nov 16, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

JelleZijlstra requested changes Nov 18, 2021

View reviewed changes

Merge branch 'master' into patch-1

2471060

JelleZijlstra approved these changes Dec 1, 2021

View reviewed changes

hauntsaninja merged commit 7e22a9e into python:master Dec 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix datetime.strftime #6317

Fix datetime.strftime #6317

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

	def strftime(self, format: str) -> str: ...
	def strftime(self, __format: str) -> str: ...

Fix datetime.strftime #6317

Fix datetime.strftime #6317

Conversation

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment