Skip to content

Conversation

@orsonadams
Copy link
Contributor

@orsonadams orsonadams commented Apr 14, 2018

Summary of changes

Return None if the user would like to ignore the timezone in the date parse.
Fix typo in warning in _build_tzaware

This is a WIP on #661.

Closes #661

Pull Request Checklist

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

@orsonadams orsonadams changed the title 661 tzinfo could be none 661 tzinfos could be None in call to parse Apr 14, 2018
@pganssle
Copy link
Member

@ParseThis This PR came in just before we added a PR template with further instructions. I edited your comment into the new PR template.

There are a few housekeeping items left. You mention that this is a WIP - is there more to do other than the changelog and AUTHORS.md?

tzdata = tzinfos(tzname, tzoffset)
else:
tzdata = tzinfos.get(tzname)
# user opted to not pass in tzinfos
Copy link
Member

Choose a reason for hiding this comment

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

I don't think "user opted not to pass in tzinfos" is correct here. I believe this line is only reached if you explicitly pass something that returns None.

# or tzinfos is set to None e.g { 'BRST' : None}
# don't punish them.
if tzdata is None:
return
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to avoid multiple returns if possible. I would probably refactor this by changing line 1124 below to:

if isinstance(tzdata, datetime.tzinfo) or tzdata is None:

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

self.assertEqual(parse("Feb 2008", default=datetime(2010, 1, 31)),
datetime(2008, 2, 29))

def testTzinfoCouldbeNone(self):
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add a separate test that checks that lambda *args: None also works without raising an error?

"Pass `tzinfos` argument in order to correctly "
"return a timezone-aware datetime. In a future "
"version, this raise an "
"version, this will raise an "
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch.

@orsonadams
Copy link
Contributor Author

@pganssle Nothing else, since its my first PR I wanted to sure of the standard here. Thanks for editing the comments! Everything else you mentioned makes sense and will be added.

This commit allows the user to specify None
when the dont care about the time zone information
in the date time parse.
@pganssle pganssle force-pushed the 661-tzinfo-could-be-none branch from 5ba764e to 5b98933 Compare April 20, 2018 00:39
@pganssle
Copy link
Member

This looks good to me. I cleaned up the history a bit, but once the tests pass I'll merge this.

Thanks for your work on this!

@pganssle pganssle merged commit 04a90b5 into dateutil:master Apr 20, 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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants