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

gh-69142: add %:z strftime format code #95983

Merged
merged 17 commits into from
Aug 28, 2022

Conversation

ThomasWaldmann
Copy link
Contributor

@ThomasWaldmann ThomasWaldmann commented Aug 14, 2022

datetime.isoformat generates the tzoffset with colons, but there
was no format code to make strftime output the same format.

for simplicity and consistency the %:z formatting behaves mostly
as %z, with the exception of adding colons. this includes the
dynamic behaviour of adding seconds and microseconds only when
needed (when not 0).

this fixes the still open "generate" part of this issue:

#69142

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

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Aug 14, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Very elegant, and the code LGTM. But would you be so kind as to add a few unit tests?

@ThomasWaldmann
Copy link
Contributor Author

@gvanrossum thanks. i even tried to start with unit tests, but it looked hard to fit in the existing test code.

but i'll try again... :-)

@merwok
Copy link
Member

merwok commented Aug 14, 2022

Here are the tests for %Z: https://github.com/python/cpython/blob/main/Lib/test/datetimetester.py#L1453

You could add another method, create the datetime instances that you need, then compare method calls with expected outputs.

datetime.isoformat generates the tzoffset with colons, but there
was no format code to make strftime output the same format.

for simplicity and consistency the %:z formatting behaves mostly
as %z, with the exception of adding colons. this includes the
dynamic behaviour of adding seconds and microseconds only when
needed (when not 0).

this fixes the still open "generate" part of this issue:

python#69142
@ThomasWaldmann ThomasWaldmann force-pushed the datetime-utcoffset-with-colon branch from d45e368 to 7d95910 Compare August 14, 2022 21:23
@ThomasWaldmann
Copy link
Contributor Author

@merwok thanks.

@gvanrossum first attempt was very elegant / short, but incorrect:

  • pin pointer was not advanced correctly for naive datetime
  • code needs 2 different replacement variables for %z and %:z (otherwise output is wrong if both formats are used in the same format string)

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I haven't tried to look into why the tests still fail. You know you can run tests locally right?

Also, please don't force-push (which I think you've done) once the review is under way. We'll squash-merge in the end anyway.

Modules/_datetimemodule.c Show resolved Hide resolved
Modules/_datetimemodule.c Outdated Show resolved Hide resolved
@merwok
Copy link
Member

merwok commented Aug 15, 2022

Please avoid force pushes for Python PRs: https://devguide.python.org/getting-started/pull-request-lifecycle/index.html

@ThomasWaldmann
Copy link
Contributor Author

ThomasWaldmann commented Aug 15, 2022

Of course I ran the tests locally before pushing (but maybe in an unusual way), so there is this interesting finding (on macOS 12, in case it matters):

% ./python.exe Lib/test/datetimetester.py -v  # all works (including the test fails seen in next command)
% ./python.exe -m test -v test_datetime # test_datetime fails

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

There seems to be still something failing. Do you need help figuring that out?

(as seen for the other replacements)
@ThomasWaldmann
Copy link
Contributor Author

ThomasWaldmann commented Aug 15, 2022

@gvanrossum yes, i could not find the cause for the strange behaviour yet.

Update:

Looks like _Fast tests are ok vs _Pure tests are broken.

Ah, just found Lib/datetime.py is the pure python implementation that also needs adapting.

@merwok merwok added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 15, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @merwok for commit 8f29d9d 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 15, 2022
@ThomasWaldmann
Copy link
Contributor Author

That patchcheck automation is not very helpful. It does not show errors, nor line numbers, just recommends to run make patchcheck locally, which is broken for me:

tw@mba2020 cpython % make patchcheck
Checked 109 modules (30 built-in, 77 shared, 2 n/a on macosx-12.5-arm64, 0 disabled, 0 missing, 0 failed on import)
./python.exe ./Tools/scripts/patchcheck.py
Enter passphrase for key '...': 
upstream/main
Getting the list of files that have been added/changed ... fatal: ambiguous argument 'upstream/main': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
error running git diff --name-status upstream/main
make: *** [patchcheck] Error 1

@merwok
Copy link
Member

merwok commented Aug 15, 2022

it works for me, and I do have the recommended setup with an upstream origin for this repo (origin being my fork)

Doc/library/datetime.rst Outdated Show resolved Hide resolved
Modules/_datetimemodule.c Show resolved Hide resolved
Modules/_datetimemodule.c Show resolved Hide resolved
@kumaraditya303 kumaraditya303 added type-feature A feature request or enhancement 3.12 bugs and security fixes labels Aug 27, 2022
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Getting very close. I have some nits to pick about the docs.

@kumaraditya303 Have you reviewed datetime.py and is it to your liking?

Doc/library/datetime.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

Have you reviewed datetime.py and is it to your liking?

LGTM

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Yes! I'll merge. Thanks TW for the PR and thanks Kumar for all the review work you did.

@ThomasWaldmann
Copy link
Contributor Author

BTW, will we have to wait some years for this until 3.12 becomes minimum requirement or can this go into older pythons also?

@gvanrossum
Copy link
Member

BTW, will we have to wait some years for this until 3.12 becomes minimum requirement or can this go into older pythons also?

Alas, this looks like a new feature to me so it won't be backported.

@ThomasWaldmann
Copy link
Contributor Author

OK, pity. OTOH, even if it were backported, it would only go into 3.11.latest, so would not help if somebody has 3.11.lessthanthat.

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x Debian 3.x has failed when building commit 023c51d.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/49/builds/3543) and take a look at the build logs.
  4. Check if the failure is related to this commit (023c51d) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/49/builds/3543

Failed tests:

  • test_multiprocessing_forkserver

Summary of the results of the build (if available):

== Tests result: FAILURE then ENV CHANGED ==

420 tests OK.

10 slowest tests:

  • test_concurrent_futures: 2 min 27 sec
  • test_tools: 2 min 17 sec
  • test_multiprocessing_spawn: 1 min 58 sec
  • test_tokenize: 1 min 24 sec
  • test_asyncio: 1 min 19 sec
  • test_gdb: 1 min 12 sec
  • test_multiprocessing_fork: 1 min 10 sec
  • test_signal: 1 min 10 sec
  • test_capi: 1 min 6 sec
  • test_unparse: 41.5 sec

1 test altered the execution environment:
test_asyncio

14 tests skipped:
test_dbm_ndbm test_devpoll test_ioctl test_kqueue test_launcher
test_msilib test_startfile test_tix test_tkinter test_ttk
test_winconsoleio test_winreg test_winsound test_zipfile64

1 re-run test:
test_multiprocessing_forkserver

Total duration: 19 min 40 sec

Click to see traceback logs
remote: Enumerating objects: 16, done.        
remote: Counting objects:   6% (1/15)        
remote: Counting objects:  13% (2/15)        
remote: Counting objects:  20% (3/15)        
remote: Counting objects:  26% (4/15)        
remote: Counting objects:  33% (5/15)        
remote: Counting objects:  40% (6/15)        
remote: Counting objects:  46% (7/15)        
remote: Counting objects:  53% (8/15)        
remote: Counting objects:  60% (9/15)        
remote: Counting objects:  66% (10/15)        
remote: Counting objects:  73% (11/15)        
remote: Counting objects:  80% (12/15)        
remote: Counting objects:  86% (13/15)        
remote: Counting objects:  93% (14/15)        
remote: Counting objects: 100% (15/15)        
remote: Counting objects: 100% (15/15), done.        
remote: Compressing objects:   7% (1/13)        
remote: Compressing objects:  15% (2/13)        
remote: Compressing objects:  23% (3/13)        
remote: Compressing objects:  30% (4/13)        
remote: Compressing objects:  38% (5/13)        
remote: Compressing objects:  46% (6/13)        
remote: Compressing objects:  53% (7/13)        
remote: Compressing objects:  61% (8/13)        
remote: Compressing objects:  69% (9/13)        
remote: Compressing objects:  76% (10/13)        
remote: Compressing objects:  84% (11/13)        
remote: Compressing objects:  92% (12/13)        
remote: Compressing objects: 100% (13/13)        
remote: Compressing objects: 100% (13/13), done.        
remote: Total 16 (delta 2), reused 3 (delta 2), pack-reused 1        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '023c51d9d8e2fd91069eea2bf12e373f1c71e9d2'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 023c51d9d8 gh-69142: add %:z strftime format code (gh-95983)
Switched to and reset branch 'main'

make: *** [Makefile:1865: buildbottest] Error 3

@gvanrossum
Copy link
Member

I don't see how this change could break test_multiprocessing_forkserver. From the logs it appears that the first time this test ran it exceeded the 15 minute time limit. The second time it ran fine. Let's ignore this unless the buildbot police get involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants