Hypothesis tests on convertyear #674
Conversation
dateutil/parser/_parser.py
Outdated
| from .. import relativedelta | ||
| from .. import tz | ||
| #from .. import relativedelta | ||
| #from .. import tz |
There was a problem hiding this comment.
Please don't leave commented-out code
dateutil/parser/_parser.py
Outdated
| ''' | ||
| Converts two-digit years to year within [-50, 49] | ||
| range of self._year (current local time) | ||
| ''' |
dateutil/parser/_parser.py
Outdated
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Don't add these new whitespace lines. pep8 convention is 1 blank line between methods.
dateutil/parser/_parser.py
Outdated
| if year - self._year >= 50: # if too far in future | ||
| year -= 100 | ||
| elif year - self._year < -50: # if too far in past | ||
| year += 100 |
There was a problem hiding this comment.
It looks like the main difference from the old version is in the case with year - self.year == -50. Is that intentional? (possibly clarified in the tests, which I haven't read yet)
There was a problem hiding this comment.
Yes, that's intentional. Spoke to Paul about the intended behavior, after testing with Hypothesis found some edge cases. Should otherwise be identical behavior as previous version, with the exception of asserting non-negative input.
There was a problem hiding this comment.
@jbrockmendel Yeah, so that someone can see my reasoning here, for the boundary of years exactly 50 years ago I think the "tie" should go to the past, not the future, since it's much more likely that you'll be parsing a specific date 50 years in the past than 50 years in the future.
dateutil/parser/_parser.py
Outdated
| # assume current century to start | ||
| year += self._century | ||
|
|
||
| if year - self._year >= 50: # if too far in future |
There was a problem hiding this comment.
pep8 two spaces before an inline comment
dateutil/test/test_parser.py
Outdated
| p = _parser.parserinfo() | ||
| new_year = p._year + n | ||
| result = p.convertyear(new_year % 100, century_specified=False) | ||
| self.assertEqual(result, new_year) |
There was a problem hiding this comment.
@pganssle is there a preferred convention for how to do these assertions? I tend to prefer just assert result == new_year
There was a problem hiding this comment.
@jbrockmendel Yeah, these should be refactored as pytest tests. This was part of a sprint event and I told @coreygirard that it was fine to write them as UnitTest tests and I'll refactor them into pytest tests before merging.
dateutil/test/test_parser.py
Outdated
| @given(integers(max_value=-1)) | ||
| def test_assert_non_negative(self, n): | ||
| with self.assertRaises(AssertionError): | ||
| _parser.parserinfo().convertyear(n) |
There was a problem hiding this comment.
_parser is a private module and should not be directly tested. Only test the public interface, so dateutil.parser.parserinfo.
Some reformatting and minor refactoring Co-Authored-By: Lauren Oldja <loldja@users.noreply.github.com>
Added assertion that year is always positive. Co-authored-by: Lauren Oldja <loldja@users.noreply.github.com>
Co-authored-by: Lauren Oldja <loldja@users.noreply.github.com>
|
I've moved the hypothesis tests into their own separate property testing folder, though it's possible we could do this with marks instead, I'm not entirely sure what the best arrangement is. I also reverted the way that |
Added property-based tests to convertyear in dateutil/parser/_parser.py
Refactored convertyear