Repository navigation
Conversation
F0RRZZ
changed the title
datetime.strftimedatetime.strftime
|
!buildbot iOS |
This comment was marked as outdated.
This comment was marked as outdated.
|
The implementation looks reasonable, but this should also be covered in the documentation, along with the fact that the |
67ec54d to
eb5c883
Compare
|
@mhsmith I adapted the tests for iOS and added documentation in the |
|
@StanFromIreland Made all changes |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
|
@mhsmith All done, but TSan seems to be stuck |
|
🤖 New build scheduled with the buildbot fleet by @mhsmith for commit 9212a5a 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F139088%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
|
@pganssle Just a gentle ping on this PR review🙏 |
|
This PR is stale because it has been open for 30 days with no activity. |
|
@StanFromIreland gentle ping |
github-actions
Bot
removed
the
stale
@StanFromIreland Since we decided to split this task into two PRs, could you please reopen this one? |
Documentation build overview
61 files changed ·
|
Co-authored-by: Muse Code (Meta Muse Spark)
On Windows and Android, every `%-X` was emulated by formatting `%X` and stripping leading zeros. That misbehaved for anything outside the documented numeric codes: non-ASCII was narrowed to a byte (`%-Ť` formatted the day, `%-é` raised OverflowError), `%-%` stuffed an unescaped `%` into the format (`%-%d` gave the day), and `%-z`/`%-Z` bypassed datetime's tzinfo handling. Emulate only `%-d`, `%-m`, `%-H`, `%-I`, `%-M`, `%-S`, `%-j`, `%-U`, `%-W`, `%-V` and `%-y`, and leave everything else to the platform as before. Gate the pure-Python branch the same way, so it no longer consumes `%-%` as a unit on other platforms, which made `%-%f` differ from the C implementation. Use the cached time.strftime instead of importing it on every call. Co-authored-by: Muse Code (Meta Muse Spark)
|
@ambv, this will require some careful review before merging, and Buildbots will be handy too. |
|
@StanFromIreland agreed! My changes are effect of my review. I'll get somebody at the sprint to look at it before we land it. |
ambv
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @ambv for commit f74ad49 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F139088%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
Emulate documented numeric dash flags on OpenBSD and account for NetBSD's native zero padding of the short year. Cover 12-hour formatting at midnight, noon and single-digit hours, document the version introducing support, and include datetime.time.strftime in the NEWS entry. Co-authored-by: Muse Code (Meta Muse Spark)
gpshead
left a comment
gpshead
left a comment
There was a problem hiding this comment.
@ambv asked me to run this through Claude as a font-of-other-platforms-knowledge reviewer looking for defects. findings:
- Test gating by platform.system() is wrong for some Apple platforms (Lib/test/datetimetester.py:1603). platform.system() returns 'iPadOS' on iPads, and tvOS/watchOS/visionOS have their own names too. Those fall into the else branch expecting
y:5, where Apple libc givesy:05. DragonFly (FreeBSD-derived libc) likely has the same problem, though I didn't check it. support.is_apple or sys.platform.startswith(('freebsd', 'netbsd')) would be safer and would match the sys.platform gating in _pydatetime.py. platform conditionals are a mess and often need abstracting (what do we have in test.support for these? -gps) - Hardcoded platform list instead of a probe. The Windows/Android/OpenBSD list is duplicated in C (#if) and Python (sys.platform). The new tests assert
%-j %-U %-W %-V,%-H %-M %-Sand%-Iunconditionally, so any other libc without the - flag fails them. AIX and Solaris are the candidates; I did not verify either. A one-time probe in the style of _need_normalize_century() (doestime.strftime('%-d', …)give'1'?) would remove the list. - NEWS overstates what the change does. It says "standardized … across all platforms", but the docs and tests in the same PR carve out
%-ystaying zero-padded on Apple, FreeBSD and NetBSD. Either emulate%-ythere too, or soften the NEWS wording. - My own human review comment added below is half addressed (datetimetester.py:1602). The docs describe the %-y inconsistency, but the test just says
# See gh-137165above the platform branch. It needs a comment there saying why expectations differ. - Doc placement. The new paragraph about
strftimeis appended to note (9), which is thestrptimeleading-zero note. It reads oddly there and probably deserves its own note, or at least a clear separation. - Edge case:
%-%-d. After an un-emulated%-, the scanner does not consume the next character as the conversion. So%-%-dbecomes%-2under emulation, where glibc natively gives%-d. This is the same existing quirk the new test deliberately pins for%-%f, so it is low priority. - Nit: import platform is appended after import warnings, out of alphabetical order; it goes away if finding 1 is fixed with
sys.platform.
| self.assertEqual(t.strftime(""), "") # SF bug #761337 | ||
| self.assertEqual(t.strftime('x'*1000), 'x'*1000) # SF bug #1556784 | ||
|
|
||
| # See gh-137165 |
There was a problem hiding this comment.
the issue doesn't explain why there should be different strftime calls and expectations based on platform. describe that in here, and make sure we document this inconsistency in behavior across platforms as intentional and let users know what they should expect.
Previously, using non-zero-padded directives (e.g.,
%-d) instrftimecaused test failures on Windows and Android (x86_64), while other platforms worked as expected.This has been fixed by manually handling such specifiers on these platforms.
Corresponding tests have been added to strftime tests.
On Windows using non-zero-padded directives raised ValueError.
On Android (x86_64) dicrectives were not replaced:
