Skip to content

Conversation

@pganssle
Copy link
Member

@pganssle pganssle commented Apr 16, 2018

Summary of changes

Tests the property that dateutil.parser.parse will support inversion of dt.isoformat() in the somewhat limiting case that sep is an ASCII character.

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See CONTRIBUTING.md for details

@pganssle pganssle added this to the Master milestone Apr 16, 2018
@pganssle pganssle requested a review from jbrockmendel April 16, 2018 15:50
@pganssle pganssle force-pushed the isoparser_hypothesis branch 2 times, most recently from 7b5c5d2 to f038715 Compare April 16, 2018 18:47
from hypothesis import given, assume
from hypothesis import strategies as st

from datetime import timedelta
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is unused.


@pytest.mark.isoparser
@given(dt=st.datetimes(timezones=TIME_ZONE_STRATEGY), sep=ASCII_STRATEGY)
def test_auto(dt, sep):
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the round-trip test, seems like an ideal use case for hypothesis. Only question is about the name test_auto. Can that be clarified? I'm guessing the isoparser marker is intended to pull some of the weight in describing the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'll change it to test_timespec_auto, though some of the weight is pulled by the isoparser mark but more by the fact that it's a test in test_isoparser_prop.py

@pganssle pganssle force-pushed the isoparser_hypothesis branch from f038715 to 8865384 Compare April 19, 2018 14:21
@pganssle
Copy link
Member Author

@jbrockmendel Should I merge?

@jbrockmendel
Copy link
Contributor

Go for it.

@pganssle pganssle merged commit 194a8e8 into dateutil:master Apr 19, 2018
@pganssle pganssle mentioned this pull request May 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants