Removed support for YYYYMM #573
Conversation
dateutil/parser/isoparser.py
Outdated
|
|
||
| if pos >= len_str: | ||
| return components, pos | ||
| raise ValueError('YYYYMM or YYYY-MM is an invalid ISO 8601 format') |
There was a problem hiding this comment.
YYYY-MM is very valid. It is only YYYYMM that is not valid (because it creates an ambiguity between ISO-8601 dates and non-ISO8601 YYMMDD triplets.
pganssle
left a comment
There was a problem hiding this comment.
I think you've misunderstood the problem. If you'd like I can write the xfail tests and you can just code against them.
| return list(o) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('d,dt_fmt', __make_date_examples()) |
There was a problem hiding this comment.
I removed this test because it's testing date strings in the format "YYYY-MM" and I thought that format was invalid as well. I'll add it back.
Just to make sure I've understood the problem correctly, date strings in the format "YYYYMM" should raise a ValueError right?
dateutil/test/test_isoparser.py
Outdated
| ('2013-02-29', ValueError), # Not a leap year | ||
| ('2014/12/03', ValueError), # Wrong separators | ||
| ('2014-04-19T', ValueError), # Unknown components | ||
| ('2012-02', ValueError) # Invalid ISO 8601 format |
There was a problem hiding this comment.
This is valid, only 201202 is invalid.
pganssle
left a comment
There was a problem hiding this comment.
This looks mostly fine. If you want to clean up the one test that is running twice that would be ideal, but I can also just do that myself if you'd prefer.
Thanks for the PR!
dateutil/test/test_isoparser.py
Outdated
| @pytest.mark.parametrize('dt', tuple(DATES)) | ||
| @pytest.mark.parametrize('fmt', | ||
| ['%Y%m', '%Y-%m']) | ||
| ['%Y-%m', '%Y-%m']) |
There was a problem hiding this comment.
I think this just repeats the same test twice. Since there's only one valid year-month format, I think you can actually move fmt into the function definition.
dateutil/test/test_isoparser.py
Outdated
| dates_no_day.append(date(1000, 11, 1)) | ||
|
|
||
| date_no_day_fmts = ('%Y%m', '%Y-%m') | ||
| date_no_day_fmts = ['%Y-%m'] |
There was a problem hiding this comment.
Why the change from tuple to list? I'm not married to using a tuple, just curious. (Though also this might be a situation where we can drop this parameter entirely, I'd have to read the tests more thoroughly to see).
There was a problem hiding this comment.
In the __make_data_examples() function o = it.product(dates_no_day, date_no_day_fmts) is used to create the examples, and date_no_day_fmts is a tuple with the two formats we want the date to be in. However, now we need only one format so a list with a singular element is used. This is similar to line 383.
There was a problem hiding this comment.
@kirit93 I don't understand why this would make you switch to a list, though.
>>>import itertools as it
>>>from datetime import date
>>>dates_no_day = [date(1999, 12, 1), date(2016, 2, 1)]
>>>dates_no_date_fmts = ('%Y-%m',)
>>>o = it.product(dates_no_day, dates_no_date_fmts)
>>>print(list(o))
[(datetime.date(1999, 12, 1), '%Y-%m'), (datetime.date(2016, 2, 1), '%Y-%m')]There was a problem hiding this comment.
You're right, I could have left it as a tuple. I just changed it to be a list because that's how it was done in the test on line 383, no other reason.
|
@kirit93 I just re-wrote that test anyway, since I've squashed a bunch of your intermediate commits and added you to the authors file. Please review the |
|
|
Fixes #546