Add timedelta strategy. Closes #333 #352
Conversation
| @@ -70,6 +70,40 @@ def do_draw(self, data): | |||
| pass | |||
|
|
|||
|
|
|||
| class TimedeltaStrategy(SearchStrategy): | |||
|
|
|||
| def __init__(self, min_days=None, max_days=None): | |||
There was a problem hiding this comment.
I think having day-range here is not good enough. How to make timedeltas for minute-wide range? Hour range? I think here better would be to have some min_value, max_value and units or mimic timedelta constructor interface to accept all those minutes, hours, days arguments (actually only one of these at once) as two value tuple of min and max value.
Day-wide timedelta is quite too broad.
There was a problem hiding this comment.
I see some problems with allowing to configure more than min/max days.
tl;dr I see possible inconsistent api (thanks to timedeltas api) and inconsistent requirements for the end user to do the math in one places, and not to do in others.
First of all, timedeltas do heavy normalization and provide only three attributes to work with: days, seconds and microseconds. If we allow to provide more than that, we will have no simple way to test if it's working properly - we will have to deal with this normalization to three attributes. I'd rather ask the end user to do the math if they want to.
Secondly, if the end user will want to filter out some values, they won't be able to do something like timedeltas(hours=(9, 18)).filter(lambda x: is_traffic_spike(x.hours)) because there is no hours attribute - they will need to do the math anyway.
|
First off: Thanks for this! Generally looks good. I think I agree with @kxepal that more granular min_value and max_value would be better (datetimes should have that too, probably). timedeltas define an ordering, so even if they don't expose the values you can still do something like I don't think I understand the second issue you raised. Could you elaborate? BTW the PR is currently failing the 100% coverage test: https://travis-ci.org/HypothesisWorks/hypothesis-python/jobs/150975122#L1450 I think this is just because timedelta doesn't actually throw exceptions in construction with any of the values you pass in - it's needed with datetime because there are weird boundary edge cases (e.g. trying to construct december 31st or February 29th on a non-leap year), so you can just remove the exception handling and retry code. |
| ), | ||
| seconds=cu.integer_range( | ||
| data, self.min_value.seconds, self.max_value.seconds | ||
| ), |
There was a problem hiding this comment.
This (and the microseconds version too) isn't actually the right logic. For example, if I have min_value=timedelta(days=0, seconds=40) then timdelta(days=1, seconds=0) is perfectly valid.
I wonder if you'd be better off doing the whole computation in microseconds - just draw an integer number of microseconds from the base and add that to self.min_value.
No description provided.