Fix datetime.strftime by karolyi · Pull Request #6317 · python/typeshed · GitHub

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

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

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'

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

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.

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.

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.

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.

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 ...)

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)

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

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

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.

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?

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.

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

thereifixedit™

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).

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

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.

Suggested change

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

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

                   def day(self) -> int: ...
                   def ctime(self) -> str: ...
-                  def strftime(self, fmt: str) -> str: ...
+                  def strftime(self, format: str) -> str: ...

                       @classmethod
                       def fromisoformat(cls: Type[_S], time_string: str) -> _S: ...
-                  def strftime(self, fmt: str) -> str: ...
+                  def strftime(self, format: str) -> str: ...

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

Fix datetime.strftime by karolyi · Pull Request #6317 · python/typeshed · GitHub

Repository navigation

Fix datetime.strftime - #6317

Fix datetime.strftime#6317hauntsaninja merged 4 commits intopython:masterpython/typeshed:masterfrom karolyi:patch-1karolyi/typeshed:patch-1Copy head branch name to clipboard

Conversation

karolyi commented Nov 16, 2021

Uh oh!

karolyi commented Nov 16, 2021

Uh oh!

This comment has been minimized.

AlexWaygood commented Nov 16, 2021

Uh oh!

This comment has been minimized.

srittau commented Nov 16, 2021

Uh oh!

karolyi commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Akuli commented Nov 16, 2021

Uh oh!

AlexWaygood commented Nov 16, 2021

Uh oh!

srittau commented Nov 16, 2021

Uh oh!

karolyi commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood commented Nov 16, 2021

Uh oh!

karolyi commented Nov 16, 2021

Uh oh!

srittau commented Nov 16, 2021

Uh oh!

karolyi commented Nov 16, 2021

Uh oh!

AlexWaygood commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srittau left a comment

Choose a reason for hiding this comment

Uh oh!

karolyi commented Nov 16, 2021

Uh oh!

hauntsaninja commented Nov 16, 2021

Uh oh!

This comment has been minimized.

Akuli commented Nov 17, 2021

Uh oh!

JelleZijlstra Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

karolyi Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Dec 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Fix datetime.strftime#6317
hauntsaninja merged 4 commits into
python:masterfrom
karolyi:patch-1

karolyi commented Nov 16, 2021 •
edited
Loading

karolyi commented Nov 16, 2021 •
edited
Loading

AlexWaygood commented Nov 16, 2021 •
edited
Loading

karolyi Nov 18, 2021 •
edited
Loading