bpo-40536: Add zoneinfo.available_timezones #20158
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was not specified in the PEP, but it will likely be a frequently requested feature if it's not included.
Member
Author
|
@FFY00 Do you mind reviewing this version? @pablogsal was going to do it, but something came up. |
FFY00
reviewed
May 17, 2020
Lib/test/test_zoneinfo/_support.py
Outdated
Comment on lines
73
to
77
| if not modname.startswith("tzdata"): | ||
| continue | ||
|
|
||
| if len(modname) > 6 and modname[7] != ".": | ||
| continue |
Member
There was a problem hiding this comment.
modname[7] != "."Did you mean modname[6]?
>>> modname = 'tzdata.something'
>>> modname[7]
's'
>>> modname[6]
'.'And just a nitpick, but maybe
if modname.split('.', maxsplit=1)[0] != 'tzdata':
continue
Member
Author
There was a problem hiding this comment.
Good call, I like your version much better. Thanks!
FFY00
reviewed
May 17, 2020
Comment on lines
+122
to
+125
| for zone in f: | ||
| zone = zone.strip() | ||
| if zone: | ||
| valid_zones.add(zone) |
Member
There was a problem hiding this comment.
I am not sure if it's better, but it could be replaced with
valid_zones.update(filter(None, map(str.strip, f)))
Member
Author
There was a problem hiding this comment.
I personally like using these more functional constructs like maps and filters, but I don't think this will make it appreciably faster (probably a little, but I'm guessing it won't shave off more than a few µs) and I think most people would find it harder to read and understand the filter-map version.
Also, for debugging reasons, it's nice when the different actions are all separate lines.
I think if this were a smaller function I'd probably go with your version.
FFY00
reviewed
May 17, 2020
Lib/test/test_zoneinfo/_support.py
Outdated
Comment on lines
73
to
74
| if not modname.startswith("tzdata"): | ||
| continue |
Member
There was a problem hiding this comment.
This can now be removed since modname.split(".", 1)[0] != "tzdata" also handles it.
Member
Author
|
Ok, I'm going to merge this so it will be ready for the beta. I'll leave it as a "release blocker" and if @ambv thinks we should revert for 3.9 we'll push it to 3.10. |
|
Member
Author
|
It's hard to tell, but I think the buildbot failure is flakiness unrelated to this change; the zoneinfo tests are passing. The build after it succeeded, so I don't think there's anything to worry about. |
Member
|
This is a known issue. You are hitting https://bugs.python.org/issue40500. |
arturoescaip
pushed a commit
to arturoescaip/cpython
that referenced
this pull request
May 24, 2020
This was not specified in the PEP, but it will likely be a frequently requested feature if it's not included. This includes only the "canonical" zones, not a simple listing of every valid value of `key` that can be passed to `Zoneinfo`, because it seems likely that that's what people will want.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was not specified in the PEP, but it will likely be a frequently requested feature if it's not included.
@ambv Given the uncertainty about the stability of this feature (we didn't have a lot of time to design it and it ends up being slightly a more complicated UI problem than one would think), please let me know ASAP if you'd like to exclude this from the beta period and push it to 3.10.
https://bugs.python.org/issue40536