Increase Test coverage for IsoParser to 100% and Pep8 #743
Conversation
Created separate branch for my test coverage changes, so they can be reviewed/merged seperately
| ('2014_W01-1', ValueError), # Invalid separator | ||
| ('2014W01-1', ValueError), # Inconsistent use of dashes | ||
| ('2014-W011', ValueError), # Inconsistent use of dashes | ||
| ('201', ValueError), # ISO string too short |
There was a problem hiding this comment.
I prefer the original comment alignment. PEP 8 does not require them to not be aligned.
| @@ -274,7 +312,7 @@ def test_iso_raises_sep(sep_act, valid_sep): | |||
|
|
|||
| @pytest.mark.xfail() | |||
| @pytest.mark.parametrize('isostr,exception', [ | |||
| ('20120425T01:2000', ValueError), # Inconsistent time separators | |||
| ('20120425T01:2000', ValueError), # Inconsistent time separators | |||
dateutil/test/test_isoparser.py
Outdated
| @@ -283,7 +321,6 @@ def test_iso_raises_failing(isostr, exception): | |||
| isoparse(isostr) | |||
|
|
|||
|
|
|||
| ### | |||
There was a problem hiding this comment.
Don't delete these "header" comments.
There was a problem hiding this comment.
The pep8-compliant way to do this I've seen elsewhere is # --------------------------------------
|
|
Sure, thanks for the feedback, you want me to resubmit with just the test case? If desired I can keep the PEP8 fixes and remove my changes that touched comment spacing and ### headers. |
|
@alimcmaster1 Doesn't have to be resubmitted, but whatever's easiest, just as long as the PEP 8 changes are in a separate commit from the substantive changes. You can rewrite the history and use |
dateutil/test/test_isoparser.py
Outdated
| TZOFFSETS))) | ||
| ((None, time(0), None),) + tuple(('%H:%M:%S.%f', _t, _tz) | ||
| for _t, _tz in | ||
| it.product([time(0), time(9, 30), time(14, 47)], TZOFFSETS))) |
There was a problem hiding this comment.
wont pep8 complain about a long line here (E501 I think)?
dateutil/test/test_isoparser.py
Outdated
| ]) | ||
| def test_isotime_raises_xfail(isostr, exception): | ||
| iparser = isoparser() | ||
| with pytest.raises(exception): | ||
| iparser.parse_isotime(isostr) | ||
| iparser.parse_isotime(isostr) |
There was a problem hiding this comment.
pep8 complaint about missing '\n' at end of file
|
Thanks both for the feedback. Since <#745> seems to be discussing styling in general in the project I will leave my pep8 changes for now. Have revert and just submitted the additional test case I added. What do you think? |
|
Will raise a new PR with no formatting changes etc |
Summary of changes
Increase test coverage of IsoParser to 100% and PEP8
Closes <#521>