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

Adding datetimes() to the core strategies #599

Closed
Zac-HD opened this issue Apr 28, 2017 · 2 comments · Fixed by #621
Closed

Adding datetimes() to the core strategies #599

Zac-HD opened this issue Apr 28, 2017 · 2 comments · Fixed by #621
Assignees
Labels
enhancement it's not broken, but we want it to be better

Comments

@Zac-HD
Copy link
Member

Zac-HD commented Apr 28, 2017

Let's start with the problem statement: the interface for time-related strategies is not good, timedeltas are missing, and users might not know that extras exist. So here's my proposal:

Add a new datetimes core strategy, with three arguments: min, max, and a tzinfo strategy - mimicking the collections with an elements= substrategy.

datetimes(min_datetime=dt.datetime.min, 
          max_datetime=dt.datetime.max, 
          timezones=none())

This has a pleasingly orthogonal API. The bounds are of the right type to express the full resolution of the strategy (#421), and after wrestling with #556 I propose to only accept naive bounds. Conceptually it's a naive strategy that can add in tzinfo at the end, which is easier to implement and understand.

Using a SearchStrategy for the timezones argument resolves the conflicts between the old allow_naive and timezones arguments - your timezones are whatever can be drawn from the strategy, including None. It also makes it much easier to extend - to draw aware timezones, simply set e.g. timezones=sampled_from([pytz.timezone(tz) for tz in pytz.all_timezones])

Additional work, and why not to do it:

  • timedeltas, as a new and entirely separate strategy, will have a separate pull request (again).
  • dates and times are trivial to construct (datetimes().map(dt.datetime.date) and datetimes().map(lambda t: t.timetz()) respectively), so I don't think it's worth adding new strategies to the public API for them at this time.
  • An example timezones() strategy was given above. I'm not inclined to build one in until I have some experience of how it would be used. (e.g.: do people want to limit geographically, by utcoffset, etc) sampled_from(some_timezones) is OK as a placeholder, with good docs.
@alexwlchan
Copy link
Contributor

I am generally 👍 on this proposal:

  • Simplifying the API is a win, especially dropping the allow_naive argument.
  • We get to deprecate all usage of the existing datetimes strategy, which makes the logic for doing so a lot simpler. Are you using this strategy? Deprecated!
  • Moving it into the core strategies is a win.
  • I would put in the dates() and times() strategy, because it’s an easy extension for us, and makes for a more friendly API for end-users. Having map() is good, but I think of it as a slightly more advanced concept, and the more we can expose directly in the hypothesis.strategies API, the better.

@Zac-HD
Copy link
Member Author

Zac-HD commented Apr 28, 2017

super simple draft of the initial implementation. No tests, deprecation, etc, but you get the idea. It's certainly more elegant this way!

@Zac-HD Zac-HD self-assigned this May 10, 2017
@Zac-HD Zac-HD added the enhancement it's not broken, but we want it to be better label May 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement it's not broken, but we want it to be better
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants