Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add converter for ISO-formatted datetime strings #473

Merged
merged 7 commits into from Oct 4, 2019

Conversation

@SebastiaanZ
Copy link
Member

commented Oct 1, 2019

This pull requests adds a converter that automatically parses ISO-formatted datetime strings and returns a datetime.datetime object. It uses dateutil.parser.isoparse to do the heavy lifting and therefore supports the same formats.

In addition, I've added tests that "guarantee" that certain standard formats will be accepted and added these "guaranteed" formats to the docstring of the ISODate.convert method.

This pull request should make it easy to implement #458. However, I did not yet want to touch the moderation cog, since it's currently being worked on by @MarkKoz. If we merge this soon, Mark may be able to directly implement it during the work he's doing anyway, though.

Related to #458

This commit adds a converter that automatically parses ISO-formatted
datetime strings and returns a `datetime.datetime` object. It uses
`dateutil.parser.isoparse` to do the heavy lifting, so it supports
the same formats as this method.

In addition, I have added tests that ensure that it accepts certain
formats and added a description of these 'guaranteed' formats to the
`ISODate.convert` docstring.

This commit should make it easy to implement #485
Copy link
Member

left a comment

lgtm otherwise

bot/converters.py Outdated Show resolved Hide resolved
Co-Authored-By: S. Co1 <sco1@users.noreply.github.com>
@sco1
sco1 approved these changes Oct 1, 2019
@sco1 sco1 added this to Needs review in Bot Tracking Oct 1, 2019
Copy link
Member

left a comment

How should time zones be dealt with? There is some other code that expects timezone-naïve datetimes (for example, utils.time.wait_until) so we could run into trouble with that. Perhaps the better solution would be to make everything else timezone-aware too.

tests/test_converters.py Outdated Show resolved Hide resolved
bot/converters.py Outdated Show resolved Hide resolved
@SebastiaanZ

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2019

How should time zones be dealt with? There is some other code that expects timezone-naïve datetimes (for example, utils.time.wait_until) so we could run into trouble with that. Perhaps the better solution would be to make everything else timezone-aware too.

This converter does not force tzinfo at the moment. If you feed it a datetime string without a timezone specifier, it will return a naive datetime.datetime, if you feed it something that does specify a timezone, it does return a tz-aware datetime.datetime:

>>> dateutil.parser.isoparse("2019-09-01T10:10:10")
datetime.datetime(2019, 9, 1, 10, 10, 10)
>>> dateutil.parser.isoparse("2019-09-01T10:10:10Z")
datetime.datetime(2019, 9, 1, 10, 10, 10, tzinfo=tzutc())

Do you think we should restrict this to one or the other at the level of the converter?

@MarkKoz

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

This converter does not force tzinfo at the moment.

Yeah, I was aware it depends on the input. Inputs that result in timezone-aware datetimes could be problematic.

Do you think we should restrict this to one or the other at the level of the converter?

Well, maybe. But as I said, perhaps a more proper solution would be to make other things work with timezone-aware datetimes. It'd be nice to support timezones in user inputs but right now it isn't an important feature anywhere (but could be in the future).

I don't know how feasible it is to support timezone-aware datetimes everywhere. Our API already returns a Z so it results in timezone-aware datetimes. In other instances we use utcnow() which could be switched to now(datetime.timezone.utc). I can't think of where else we work with datetimes at the moment.

SebastiaanZ and others added 2 commits Oct 2, 2019
Co-Authored-By: Mark <kozlovmark@gmail.com>
This commit removes the angle brackets from the url in the docstring
of `ISODateTime.convert`. The reason: it's ugly.
@SebastiaanZ

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2019

Okay, I've looked into the option of using timezone-aware datetime objects everywhere and I like the idea. However, I think it is a much broader and larger issue than this converter and I don't think we should tack such a change onto this pull request. That's why my proposal is to make sure that this converter returns the UTC-time stored in timezone-unaware datetime objects for now.

The reason is that discord.py returns unaware datetime objects for datetime related attributes and we wouldn't be able to easily compare "our" tz-aware datetime objects with these tz-unaware datetime objects returned by discord.py. I think that it's therefore best to return tz-unaware datetime objects until we've looked into the broader topic of using tz-aware objects everywhere by default in the bot.

The parser we use, `dateutil.parsers.isoparse` returns a timezone-
aware or timezone-unaware `datetime` object depending on whether or
not the datetime string included a timezone offset specification.

Since we can't compare tz-aware objects to tz-unaware objects it's
better to make sure our converter is consistent in the type it will
return.

For now, I've chosen to return tz-unaware datetime objects, since
`discord.py` also returns tz-unaware datetime objects when accessing
datetime-related attributes of objects. Since we're likely to compare
"our" datetime objects to discord.py-provided datetime objects, I
think that's the most parsimonious option for now.

Note: It's probably a good idea to open a larger discussion about
using timezone-aware datetime objects throughout the library to
avoid a UTC-time being interpreted as localtime. This will require
a broader discussion than this commit/PR allows, though.
tests/test_converters.py Outdated Show resolved Hide resolved
As we have decided that the converter should return naive datetime
objects, we should explicitly test that datetime strings with a
timezone offset are still converted to a naive datetime object. I
have done this by adding a `tzinfo is None` assertion.
Bot Tracking automation moved this from Needs review to Reviewer approved Oct 4, 2019
@MarkKoz
MarkKoz approved these changes Oct 4, 2019
Copy link
Member

left a comment

Sorry it took me a few days to get back to this. It looks good now.

@MarkKoz MarkKoz merged commit 7695508 into master Oct 4, 2019
2 checks passed
2 checks passed
Bot Build #20191004.8 succeeded
Details
Bot (Lint & Test) Lint & Test succeeded
Details
Bot Tracking automation moved this from Reviewer approved to Done Oct 4, 2019
@MarkKoz MarkKoz deleted the ISODate-converter branch Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Bot Tracking
  
Done
3 participants
You can’t perform that action at this time.