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

st.timezones() strategy using the stdlib zoneinfo module #2682

Merged
merged 2 commits into from
Dec 9, 2020

Conversation

Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented Nov 30, 2020

From one perspective, this is a pretty standard PR adding a new strategy - it's almost identical to the hypothesis.extra.{dateutil,pytz}.timezones() strategies, but in the hypothesis.strategies namespace because the timezones in question come from the standard library.

On the other hand, they're only in the latest stdlib, so we also have some logic to manage backports - mostly through a new hypothesis[zoneinfo] extra which has only conditional dependencies. This is clearly documented and raises nice errors if you don't have the deps; and since it'll be standard eventually I'd prefer to put it in the right place to start with.

Implementation of allow_aliases and allow_deprecated arguments for timezone_keys() has been left to a future PR, along with #2414 - this is just the increment to get a minimal useful API out there. Closes #2630.

@Zac-HD Zac-HD added the new-feature entirely novel capabilities or strategies label Nov 30, 2020
@Zac-HD Zac-HD force-pushed the zoneinfo branch 6 times, most recently from 0d52c43 to 0db20ac Compare December 1, 2020 04:08
Copy link
Contributor

@PiDelport PiDelport left a comment

Choose a reason for hiding this comment

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

I added some questions, but this looks good!

@Zac-HD Zac-HD force-pushed the zoneinfo branch 4 times, most recently from 26cad9f to 2e5ad5b Compare December 9, 2020 09:06
@Zac-HD Zac-HD merged commit 775aba9 into HypothesisWorks:master Dec 9, 2020
@Zac-HD Zac-HD deleted the zoneinfo branch December 9, 2020 11:39
import zoneinfo
except ImportError:
try:
from backports import zoneinfo # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

This broke backports.zoneinfo's test suite (mainly because backports.zoneinfo's test suite relies on importing backports.zoneinfo in its own weird way.

Not that you need to change anything, just thought it's kinda funny.

@pganssle
Copy link
Contributor

Implementation of allow_aliases and allow_deprecated arguments for timezone_keys() has been left to a future PR, along with #2414 - this is just the increment to get a minimal useful API out there. Closes #2630.

It's not clear how easy it would be to implement such a thing using only the time zone data on disk. You may be able to detect if a given zone is a hard link to another zone, but I'm not actually sure if the hard links are guaranteed to point to the canonical zone (as opposed to a random one). dateutil has some sort of zone deduplication logic in the code that builds the zoneinfo tarball and it seems to consistently map America/New_York to US/Eastern rather than the other way around. I haven't tried very hard to fix that.

You can probably hard-code a list of existing deprecated zones like anything in Etc/ or US/, though. Not to discourage you from trying to find a programmatic way to detect these (I haven't spent enough time to rule out that it can be done), just might be good to temper expectations if you open an issue for this and some new contributor comes by thinking it might be easy 😛.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature entirely novel capabilities or strategies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a st.timezones() strategy using the stdlib zoneinfo module
3 participants