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

Add dates() and times() strategies to hypothesis.extra.datetime #160

Merged
merged 1 commit into from Sep 28, 2015

Conversation

@adamchainz
Copy link
Contributor

commented Sep 22, 2015

  • Add dates and times strategies
  • Add tests
  • Add documentation, improving datetimes documentation
  • Add livehtml target to docs/Makefile using sphinx-autobuild for live docs editing
  • gitignore update with some folders that appear during tests
@DRMacIver

This comment has been minimized.

Copy link
Member

commented Sep 22, 2015

Generally pretty strongly in favour of this feature, and assuming the build passes mostly looks good, thanks! I'll add some specific comments inline though.

@@ -276,6 +276,30 @@ def datetimes(allow_naive=None, timezones=None, min_year=None, max_year=None):
)


def dates(min_year=None, max_year=None):

This comment has been minimized.

Copy link
@DRMacIver

DRMacIver Sep 22, 2015

Member

hypothesis.strategies has a 'defines_strategy' decorator that should be used here that fixes up the repr of the returned strategy to correspond to the function that generates it. This should be used here and on times.

This comment has been minimized.

Copy link
@DRMacIver

DRMacIver Sep 22, 2015

Member

(I've just noticed I'm not using it on datetimes. I'll fix that)

This comment has been minimized.

Copy link
@adamchainz

adamchainz Sep 23, 2015

Author Contributor

Done!

return dt.date()


def times():

This comment has been minimized.

Copy link
@DRMacIver

DRMacIver Sep 22, 2015

Member

datetime.times does support timezones and it would be nice to expose that with the same set of arguments that are present in datetimes().

This comment has been minimized.

Copy link
@adamchainz

adamchainz Sep 23, 2015

Author Contributor

Done! 🐰

@DRMacIver

This comment has been minimized.

Copy link
Member

commented Sep 23, 2015

LGTM. Will wait for the build and then assuming everything goes according to plan shall merge.

@DRMacIver

This comment has been minimized.

Copy link
Member

commented Sep 23, 2015

Ah, you've failed the linter. If you run tox -e lint locally this will fix up most of the errors.

@DRMacIver

This comment has been minimized.

Copy link
Member

commented Sep 23, 2015

The pypy segfault is not your fault and you should ignore it. Something I've done recently is triggering an intermittent pypy segfault. This is sadly a thing that happens.

@DRMacIver

This comment has been minimized.

Copy link
Member

commented Sep 23, 2015

Also you might want to rebase this against master, as there have been a few changes (one of which affects datetime).

@adamchainz adamchainz force-pushed the adamchainz:date_and_time branch from a20c183 to b4c240b Sep 24, 2015
@adamchainz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2015

Linting and rebasing done.

@DRMacIver

This comment has been minimized.

Copy link
Member

commented Sep 24, 2015

Sorry, one final thing: You should add yourself to the list of contributors in CONTRIBUTING.rst :-)

@adamchainz adamchainz force-pushed the adamchainz:date_and_time branch from b4c240b to c70916c Sep 24, 2015
@adamchainz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2015

🎺 done

You now have TWO Adam Johnsons as contributors - suspicious...

@DRMacIver

This comment has been minimized.

Copy link
Member

commented Sep 24, 2015

Sorry, I said "finally" and then the build failed. :-(

The appveyor one is my fault (oops. Fixing now). The coverage one is I think you (not the strings case, which I've never seen fail before but looks like it can happen with very very low probability), but the datetimes one you haven't tested your argument validation for times().

One option there would be to just use the existing datetimes() function and inherit the argument validation from it rather than having to write your own.

@adamchainz adamchainz force-pushed the adamchainz:date_and_time branch from c70916c to c717e4f Sep 25, 2015
DRMacIver added a commit that referenced this pull request Sep 28, 2015
Add dates() and times() strategies to hypothesis.extra.datetime
@DRMacIver DRMacIver merged commit c07e240 into HypothesisWorks:master Sep 28, 2015
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@DRMacIver

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

Sorry, I missed the final fix on this had happened.

Thanks again for the feature!

@adamchainz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

🌈 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.