Skip to content

Add fix for b-Y-d cases; add tests; remove redundant tests#481

Merged
pganssle merged 5 commits into
dateutil:masterfrom
jbrockmendel:bYd2
Oct 27, 2017
Merged

Add fix for b-Y-d cases; add tests; remove redundant tests#481
pganssle merged 5 commits into
dateutil:masterfrom
jbrockmendel:bYd2

Conversation

@jbrockmendel

Copy link
Copy Markdown
Contributor

Replaces #441

Comment thread dateutil/test/test_parser.py Outdated
from six.moves import StringIO


def test_Ybd_permutations():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't actually test anything unless you run it with pytest (and even then it's a deprecated way to do this). The function should be moved into the TestCase object until we move over to pytest, at which point we'll just switch to the parametrized version.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note that by "moved to" I mean that it should not yield anything, and the assert statement should be switched to a self.assertEqual() call.

@jbrockmendel

jbrockmendel commented Oct 26, 2017 via email

Copy link
Copy Markdown
Contributor Author

@jbrockmendel

Copy link
Copy Markdown
Contributor Author

Just pushed; added a new test class in the hopes that we can avoid the one-giant-class pattern.

It occurs to me: these tests should be robust to "fuzzy", "dayfirst", and "yearfirst". I may follow-up by extending the test. For now let's just get this bugfix in.

@pganssle

Copy link
Copy Markdown
Member

I think the appveyor build failure might have been a temporary failure on the AppVeyor side, kicked off a new build.

Comment thread dateutil/test/test_parser.py Outdated
# terms are in and for each of the separators below.

seps = ['-', ' ', '/', '.']
token_opts = [['%Y'], ['%b', '%B'], ['%d', '%-d']]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think %-d might not be supported on Windows.

@pganssle

Copy link
Copy Markdown
Member

strftime.org says that %-d is platform specific, and I think that Windows doesn't support it, which was causing the AppVeyor failures that were happening after I fixed the unrelated TLS issue.

I've updated it now so that '%-d' tests are only run if the platform supports them, and your innermost for loop was at the wrong level (though this made no difference).

I removed the nested for loops for year, month and day because they were not necessary (we'll get back to that when we're using pytest), and also dropped the default spec, because I think it's actually dangerous to make the correct answer the default value if nothing is found for a given token.

@pganssle pganssle mentioned this pull request Oct 27, 2017
@pganssle pganssle added this to the 2.7.0 milestone Oct 27, 2017
@pganssle
pganssle merged commit 6d6dc2f into dateutil:master Oct 27, 2017
@jbrockmendel

Copy link
Copy Markdown
Contributor Author

which was causing the AppVeyor failures that were happening after I fixed the unrelated TLS issue.

Nice catch!

@pganssle pganssle mentioned this pull request Mar 11, 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