From fba165037943fda90039ec9cadf0649cfae0e781 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com> Date: Mon, 23 Sep 2019 21:13:47 +0200 Subject: Fix failing duration conversion https://github.com/python-discord/bot/issues/446 The current ExpirationDate converter does not convert duration strings to `datetime.datetime` objects correctly. To remedy the problem, I've written a new Duration converter that uses regex matching to extract the relevant duration units and `dateutil.relativedelta.relativedelta` to compute a `datetime.datetime` that's the given duration in the future. I've left the old `ExpirationDate` converter in place for now, since the new Duration converter may not be the most optimal method. However, given the importance of being able to convert durations for moderation tasks, I think it's better to implement Duration now and rethink the approach later. This commit closes #446 --- tests/test_converters.py | 82 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 78 insertions(+), 4 deletions(-) (limited to 'tests/test_converters.py') diff --git a/tests/test_converters.py b/tests/test_converters.py index 3cf774c80..3cf00035f 100644 --- a/tests/test_converters.py +++ b/tests/test_converters.py @@ -1,11 +1,12 @@ import asyncio -from datetime import datetime -from unittest.mock import MagicMock +import datetime +from unittest.mock import MagicMock, patch import pytest from discord.ext.commands import BadArgument from bot.converters import ( + Duration, ExpirationDate, TagContentConverter, TagNameConverter, @@ -17,10 +18,10 @@ from bot.converters import ( ('value', 'expected'), ( # sorry aliens - ('2199-01-01T00:00:00', datetime(2199, 1, 1)), + ('2199-01-01T00:00:00', datetime.datetime(2199, 1, 1)), ) ) -def test_expiration_date_converter_for_valid(value: str, expected: datetime): +def test_expiration_date_converter_for_valid(value: str, expected: datetime.datetime): converter = ExpirationDate() assert asyncio.run(converter.convert(None, value)) == expected @@ -91,3 +92,76 @@ def test_valid_python_identifier_for_valid(value: str): def test_valid_python_identifier_for_invalid(value: str): with pytest.raises(BadArgument, match=f'`{value}` is not a valid Python identifier'): asyncio.run(ValidPythonIdentifier.convert(None, value)) + + +FIXED_UTC_NOW = datetime.datetime.fromisoformat('2019-01-01T00:00:00') + + +@pytest.mark.parametrize( + ('duration', 'expected'), + ( + # Simple duration strings + ('1Y', datetime.datetime.fromisoformat('2020-01-01T00:00:00')), + ('1y', datetime.datetime.fromisoformat('2020-01-01T00:00:00')), + ('1year', datetime.datetime.fromisoformat('2020-01-01T00:00:00')), + ('1years', datetime.datetime.fromisoformat('2020-01-01T00:00:00')), + ('1m', datetime.datetime.fromisoformat('2019-02-01T00:00:00')), + ('1month', datetime.datetime.fromisoformat('2019-02-01T00:00:00')), + ('1months', datetime.datetime.fromisoformat('2019-02-01T00:00:00')), + ('1w', datetime.datetime.fromisoformat('2019-01-08T00:00:00')), + ('1W', datetime.datetime.fromisoformat('2019-01-08T00:00:00')), + ('1week', datetime.datetime.fromisoformat('2019-01-08T00:00:00')), + ('1weeks', datetime.datetime.fromisoformat('2019-01-08T00:00:00')), + ('1d', datetime.datetime.fromisoformat('2019-01-02T00:00:00')), + ('1D', datetime.datetime.fromisoformat('2019-01-02T00:00:00')), + ('1day', datetime.datetime.fromisoformat('2019-01-02T00:00:00')), + ('1days', datetime.datetime.fromisoformat('2019-01-02T00:00:00')), + ('1h', datetime.datetime.fromisoformat('2019-01-01T01:00:00')), + ('1H', datetime.datetime.fromisoformat('2019-01-01T01:00:00')), + ('1hour', datetime.datetime.fromisoformat('2019-01-01T01:00:00')), + ('1hours', datetime.datetime.fromisoformat('2019-01-01T01:00:00')), + ('1M', datetime.datetime.fromisoformat('2019-01-01T00:01:00')), + ('1minute', datetime.datetime.fromisoformat('2019-01-01T00:01:00')), + ('1minutes', datetime.datetime.fromisoformat('2019-01-01T00:01:00')), + ('1s', datetime.datetime.fromisoformat('2019-01-01T00:00:01')), + ('1S', datetime.datetime.fromisoformat('2019-01-01T00:00:01')), + ('1second', datetime.datetime.fromisoformat('2019-01-01T00:00:01')), + ('1seconds', datetime.datetime.fromisoformat('2019-01-01T00:00:01')), + + # Complex duration strings + ('1y1m1w1d1H1M1S', datetime.datetime.fromisoformat('2020-02-09T01:01:01')), + ('5y100S', datetime.datetime.fromisoformat('2024-01-01T00:01:40')), + ('2w28H', datetime.datetime.fromisoformat('2019-01-16T04:00:00')), + ) +) +def test_duration_converter_for_valid(duration: str, expected: datetime): + converter = Duration() + + with patch('bot.converters.datetime') as mock_datetime: + mock_datetime.utcnow.return_value = FIXED_UTC_NOW + assert asyncio.run(converter.convert(None, duration)) == expected + + +@pytest.mark.parametrize( + ('duration'), + ( + # Units in wrong order + ('1d1w'), + ('1s1y'), + + # Unknown substrings + ('1MVes'), + ('1y3breads'), + + # Missing amount + ('ym'), + + # Garbage + ('Guido van Rossum'), + ('lemon lemon lemon lemon lemon lemon lemon'), + ) +) +def test_duration_converter_for_invalid(duration: str): + converter = Duration() + with pytest.raises(BadArgument, match=f'`{duration}` is not a valid duration string.'): + asyncio.run(converter.convert(None, duration)) -- cgit v1.2.3 From 56a365b24e7f6e145718a13edc410848ab169a06 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com> Date: Mon, 23 Sep 2019 23:03:58 +0200 Subject: Allow whitespace in duration strings and update tests https://github.com/python-discord/bot/issues/446 After review feedback and a discussion in the dev-core team, I've changed a couple of things: - Allow a space between amount and unit in the duration string; - Allow a space between different units in the duration string; - Remove the old ExpirationDate converter completely; - Remove the dependency `dateparser` from the Pipfile; - Update tests for the two types of optional spaces; - Change the test for valid cases to a more readable format; This PR closes #446 --- Pipfile | 1 - Pipfile.lock | 57 ++++++------------------ bot/converters.py | 39 ++++------------ tests/test_converters.py | 113 +++++++++++++++++++++++++++-------------------- 4 files changed, 87 insertions(+), 123 deletions(-) (limited to 'tests/test_converters.py') diff --git a/Pipfile b/Pipfile index 6a58054c1..33f44e9a6 100644 --- a/Pipfile +++ b/Pipfile @@ -17,7 +17,6 @@ aio-pika = "*" python-dateutil = "*" deepdiff = "*" requests = "*" -dateparser = "*" more_itertools = "~=7.2" urllib3 = ">=1.24.2,<1.25" diff --git a/Pipfile.lock b/Pipfile.lock index 9bdcff923..7674acb26 100644 --- a/Pipfile.lock +++ b/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "29aaaa90a070d544e5b39fb6033410daa9bb7f658077205e44099f3175f6822b" + "sha256": "d582b1e226b1ce675817161d9059352d8f303c1bc1646034a9e73673f6581d12" }, "pipfile-spec": 6, "requires": { @@ -62,10 +62,10 @@ }, "aiormq": { "hashes": [ - "sha256:0b755b748d87d5ec63b4b7f162102333bf0901caf1f8a2bf29467bbdd54e637d", - "sha256:f8eef1f98bc331a266404d925745fac589dab30412688564d740754d6d643863" + "sha256:c3e4dd01a2948a75f739fb637334dbb8c6f1a4cecf74d5ed662dc3bab7f39973", + "sha256:e220d3f9477bb2959b729b79bec815148ddb8a7686fc6c3d05d41c88ebd7c59e" ], - "version": "==2.7.5" + "version": "==2.8.0" }, "alabaster": { "hashes": [ @@ -150,14 +150,6 @@ ], "version": "==3.0.4" }, - "dateparser": { - "hashes": [ - "sha256:983d84b5e3861cb0aa240cad07f12899bb10b62328aae188b9007e04ce37d665", - "sha256:e1eac8ef28de69a554d5fcdb60b172d526d61924b1a40afbbb08df459a36006b" - ], - "index": "pypi", - "version": "==0.7.2" - }, "deepdiff": { "hashes": [ "sha256:1123762580af0904621136d117c8397392a244d3ff0fa0a50de57a7939582476", @@ -342,10 +334,10 @@ }, "packaging": { "hashes": [ - "sha256:a7ac867b97fdc07ee80a8058fe4435ccd274ecc3b0ed61d852d7d53055528cf9", - "sha256:c491ca87294da7cc01902edbe30a5bc6c4c28172b5138ab4e4aa1b9d7bfaeafe" + "sha256:28b924174df7a2fa32c1953825ff29c61e2f5e082343165438812f00d3a7fc47", + "sha256:d9551545c6d761f3def1677baf08ab2a3ca17c56879e70fecba2fc4dde4ed108" ], - "version": "==19.1" + "version": "==19.2" }, "pamqp": { "hashes": [ @@ -432,22 +424,6 @@ "index": "pypi", "version": "==5.1.2" }, - "regex": { - "hashes": [ - "sha256:1e9f9bc44ca195baf0040b1938e6801d2f3409661c15fe57f8164c678cfc663f", - "sha256:587b62d48ca359d2d4f02d486f1f0aa9a20fbaf23a9d4198c4bed72ab2f6c849", - "sha256:835ccdcdc612821edf132c20aef3eaaecfb884c9454fdc480d5887562594ac61", - "sha256:93f6c9da57e704e128d90736430c5c59dd733327882b371b0cae8833106c2a21", - "sha256:a46f27d267665016acb3ec8c6046ec5eae8cf80befe85ba47f43c6f5ec636dcd", - "sha256:c5c8999b3a341b21ac2c6ec704cfcccbc50f1fedd61b6a8ee915ca7fd4b0a557", - "sha256:d4d1829cf97632673aa49f378b0a2c3925acd795148c5ace8ef854217abbee89", - "sha256:d96479257e8e4d1d7800adb26bf9c5ca5bab1648a1eddcac84d107b73dc68327", - "sha256:f20f4912daf443220436759858f96fefbfc6c6ba9e67835fd6e4e9b73582791a", - "sha256:f2b37b5b2c2a9d56d9e88efef200ec09c36c7f323f9d58d0b985a90923df386d", - "sha256:fe765b809a1f7ce642c2edeee351e7ebd84391640031ba4b60af8d91a9045890" - ], - "version": "==2019.8.19" - }, "requests": { "hashes": [ "sha256:11e007a8a2aa0323f5a921e9e6a2d7e4e67d9877e85773fba9ba6419025cbeb4", @@ -526,13 +502,6 @@ ], "version": "==1.1.3" }, - "tzlocal": { - "hashes": [ - "sha256:11c9f16e0a633b4b60e1eede97d8a46340d042e67b670b290ca526576e039048", - "sha256:949b9dd5ba4be17190a80c0268167d7e6c92c62b30026cf9764caf3e308e5590" - ], - "version": "==2.0.0" - }, "urllib3": { "hashes": [ "sha256:2393a695cd12afedd0dcb26fe5d50d0cf248e5a66f75dbd89a3d4eb333a61af4", @@ -800,10 +769,10 @@ }, "packaging": { "hashes": [ - "sha256:a7ac867b97fdc07ee80a8058fe4435ccd274ecc3b0ed61d852d7d53055528cf9", - "sha256:c491ca87294da7cc01902edbe30a5bc6c4c28172b5138ab4e4aa1b9d7bfaeafe" + "sha256:28b924174df7a2fa32c1953825ff29c61e2f5e082343165438812f00d3a7fc47", + "sha256:d9551545c6d761f3def1677baf08ab2a3ca17c56879e70fecba2fc4dde4ed108" ], - "version": "==19.1" + "version": "==19.2" }, "pluggy": { "hashes": [ @@ -857,11 +826,11 @@ }, "pytest": { "hashes": [ - "sha256:95d13143cc14174ca1a01ec68e84d76ba5d9d493ac02716fd9706c949a505210", - "sha256:b78fe2881323bd44fd9bd76e5317173d4316577e7b1cddebae9136a4495ec865" + "sha256:813b99704b22c7d377bbd756ebe56c35252bb710937b46f207100e843440b3c2", + "sha256:cc6620b96bc667a0c8d4fa592a8c9c94178a1bd6cc799dbb057dfd9286d31a31" ], "index": "pypi", - "version": "==5.1.2" + "version": "==5.1.3" }, "pytest-cov": { "hashes": [ diff --git a/bot/converters.py b/bot/converters.py index b7340982b..db3e2a426 100644 --- a/bot/converters.py +++ b/bot/converters.py @@ -4,7 +4,6 @@ from datetime import datetime from ssl import CertificateError from typing import Union -import dateparser import discord from aiohttp import ClientConnectorError from dateutil.relativedelta import relativedelta @@ -179,39 +178,17 @@ class TagContentConverter(Converter): return tag_content -class ExpirationDate(Converter): - """Convert relative expiration date into UTC datetime using dateparser.""" - - DATEPARSER_SETTINGS = { - 'PREFER_DATES_FROM': 'future', - 'TIMEZONE': 'UTC', - 'TO_TIMEZONE': 'UTC' - } - - async def convert(self, ctx: Context, expiration_string: str) -> datetime: - """Convert relative expiration date into UTC datetime.""" - expiry = dateparser.parse(expiration_string, settings=self.DATEPARSER_SETTINGS) - if expiry is None: - raise BadArgument(f"Failed to parse expiration date from `{expiration_string}`") - - now = datetime.utcnow() - if expiry < now: - expiry = now + (now - expiry) - - return expiry - - class Duration(Converter): """Convert duration strings into UTC datetime.datetime objects.""" duration_parser = re.compile( - r"((?P\d+?)(years|year|Y|y))?" - r"((?P\d+?)(months|month|m))?" - r"((?P\d+?)(weeks|week|W|w))?" - r"((?P\d+?)(days|day|D|d))?" - r"((?P\d+?)(hours|hour|H|h))?" - r"((?P\d+?)(minutes|minute|M))?" - r"((?P\d+?)(seconds|second|S|s))?" + r"((?P\d+?) ?(years|year|Y|y) ?)?" + r"((?P\d+?) ?(months|month|m) ?)?" + r"((?P\d+?) ?(weeks|week|W|w) ?)?" + r"((?P\d+?) ?(days|day|D|d) ?)?" + r"((?P\d+?) ?(hours|hour|H|h) ?)?" + r"((?P\d+?) ?(minutes|minute|M) ?)?" + r"((?P\d+?) ?(seconds|second|S|s))?" ) async def convert(self, ctx: Context, duration: str) -> datetime: @@ -227,7 +204,7 @@ class Duration(Converter): if not match: raise BadArgument(f"`{duration}` is not a valid duration string.") - duration_dict = {unit: int(amount) for unit, amount in match.groupdict().items() if amount} + duration_dict = {unit: int(amount) for unit, amount in match.groupdict(default=0).items()} delta = relativedelta(**duration_dict) now = datetime.utcnow() diff --git a/tests/test_converters.py b/tests/test_converters.py index 3cf00035f..35fc5d88e 100644 --- a/tests/test_converters.py +++ b/tests/test_converters.py @@ -3,29 +3,17 @@ import datetime from unittest.mock import MagicMock, patch import pytest +from dateutil.relativedelta import relativedelta from discord.ext.commands import BadArgument from bot.converters import ( Duration, - ExpirationDate, TagContentConverter, TagNameConverter, ValidPythonIdentifier, ) -@pytest.mark.parametrize( - ('value', 'expected'), - ( - # sorry aliens - ('2199-01-01T00:00:00', datetime.datetime(2199, 1, 1)), - ) -) -def test_expiration_date_converter_for_valid(value: str, expected: datetime.datetime): - converter = ExpirationDate() - assert asyncio.run(converter.convert(None, value)) == expected - - @pytest.mark.parametrize( ('value', 'expected'), ( @@ -97,46 +85,68 @@ def test_valid_python_identifier_for_invalid(value: str): FIXED_UTC_NOW = datetime.datetime.fromisoformat('2019-01-01T00:00:00') -@pytest.mark.parametrize( - ('duration', 'expected'), - ( +@pytest.fixture( + params=( # Simple duration strings - ('1Y', datetime.datetime.fromisoformat('2020-01-01T00:00:00')), - ('1y', datetime.datetime.fromisoformat('2020-01-01T00:00:00')), - ('1year', datetime.datetime.fromisoformat('2020-01-01T00:00:00')), - ('1years', datetime.datetime.fromisoformat('2020-01-01T00:00:00')), - ('1m', datetime.datetime.fromisoformat('2019-02-01T00:00:00')), - ('1month', datetime.datetime.fromisoformat('2019-02-01T00:00:00')), - ('1months', datetime.datetime.fromisoformat('2019-02-01T00:00:00')), - ('1w', datetime.datetime.fromisoformat('2019-01-08T00:00:00')), - ('1W', datetime.datetime.fromisoformat('2019-01-08T00:00:00')), - ('1week', datetime.datetime.fromisoformat('2019-01-08T00:00:00')), - ('1weeks', datetime.datetime.fromisoformat('2019-01-08T00:00:00')), - ('1d', datetime.datetime.fromisoformat('2019-01-02T00:00:00')), - ('1D', datetime.datetime.fromisoformat('2019-01-02T00:00:00')), - ('1day', datetime.datetime.fromisoformat('2019-01-02T00:00:00')), - ('1days', datetime.datetime.fromisoformat('2019-01-02T00:00:00')), - ('1h', datetime.datetime.fromisoformat('2019-01-01T01:00:00')), - ('1H', datetime.datetime.fromisoformat('2019-01-01T01:00:00')), - ('1hour', datetime.datetime.fromisoformat('2019-01-01T01:00:00')), - ('1hours', datetime.datetime.fromisoformat('2019-01-01T01:00:00')), - ('1M', datetime.datetime.fromisoformat('2019-01-01T00:01:00')), - ('1minute', datetime.datetime.fromisoformat('2019-01-01T00:01:00')), - ('1minutes', datetime.datetime.fromisoformat('2019-01-01T00:01:00')), - ('1s', datetime.datetime.fromisoformat('2019-01-01T00:00:01')), - ('1S', datetime.datetime.fromisoformat('2019-01-01T00:00:01')), - ('1second', datetime.datetime.fromisoformat('2019-01-01T00:00:01')), - ('1seconds', datetime.datetime.fromisoformat('2019-01-01T00:00:01')), + ('1Y', {"years": 1}), + ('1y', {"years": 1}), + ('1year', {"years": 1}), + ('1years', {"years": 1}), + ('1m', {"months": 1}), + ('1month', {"months": 1}), + ('1months', {"months": 1}), + ('1w', {"weeks": 1}), + ('1W', {"weeks": 1}), + ('1week', {"weeks": 1}), + ('1weeks', {"weeks": 1}), + ('1d', {"days": 1}), + ('1D', {"days": 1}), + ('1day', {"days": 1}), + ('1days', {"days": 1}), + ('1h', {"hours": 1}), + ('1H', {"hours": 1}), + ('1hour', {"hours": 1}), + ('1hours', {"hours": 1}), + ('1M', {"minutes": 1}), + ('1minute', {"minutes": 1}), + ('1minutes', {"minutes": 1}), + ('1s', {"seconds": 1}), + ('1S', {"seconds": 1}), + ('1second', {"seconds": 1}), + ('1seconds', {"seconds": 1}), # Complex duration strings - ('1y1m1w1d1H1M1S', datetime.datetime.fromisoformat('2020-02-09T01:01:01')), - ('5y100S', datetime.datetime.fromisoformat('2024-01-01T00:01:40')), - ('2w28H', datetime.datetime.fromisoformat('2019-01-16T04:00:00')), + ( + '1y1m1w1d1H1M1S', + { + "years": 1, + "months": 1, + "weeks": 1, + "days": 1, + "hours": 1, + "minutes": 1, + "seconds": 1 + } + ), + ('5y100S', {"years": 5, "seconds": 100}), + ('2w28H', {"weeks": 2, "hours": 28}), + + # Duration strings with spaces + ('1 year 2 months', {"years": 1, "months": 2}), + ('1d 2H', {"days": 1, "hours": 2}), + ('1 week2 days', {"weeks": 1, "days": 2}), ) ) -def test_duration_converter_for_valid(duration: str, expected: datetime): - converter = Duration() +def create_future_datetime(request): + """Yields duration string and target datetime.datetime object.""" + duration, duration_dict = request.param + future_datetime = FIXED_UTC_NOW + relativedelta(**duration_dict) + yield duration, future_datetime + +def test_duration_converter_for_valid(create_future_datetime: tuple): + converter = Duration() + duration, expected = create_future_datetime with patch('bot.converters.datetime') as mock_datetime: mock_datetime.utcnow.return_value = FIXED_UTC_NOW assert asyncio.run(converter.convert(None, duration)) == expected @@ -149,6 +159,10 @@ def test_duration_converter_for_valid(duration: str, expected: datetime): ('1d1w'), ('1s1y'), + # Duplicated units + ('1 year 2 years'), + ('1 M 10 minutes'), + # Unknown substrings ('1MVes'), ('1y3breads'), @@ -156,6 +170,11 @@ def test_duration_converter_for_valid(duration: str, expected: datetime): # Missing amount ('ym'), + # Incorrect whitespace + (" 1y"), + ("1S "), + ("1y 1m"), + # Garbage ('Guido van Rossum'), ('lemon lemon lemon lemon lemon lemon lemon'), -- cgit v1.2.3 From 20da07562aab1d9041170f70e1a3dc086f5c1b90 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com> Date: Tue, 1 Oct 2019 10:53:18 +0200 Subject: Add converter for ISO-formatted datetime strings Related to https://github.com/python-discord/bot/issues/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 --- bot/converters.py | 33 +++++++++++++++++++++++++++++ tests/test_converters.py | 55 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+) (limited to 'tests/test_converters.py') diff --git a/bot/converters.py b/bot/converters.py index 339da7b60..49ac488f4 100644 --- a/bot/converters.py +++ b/bot/converters.py @@ -4,6 +4,7 @@ from datetime import datetime from ssl import CertificateError from typing import Union +import dateutil.parser import discord from aiohttp import ClientConnectorError from dateutil.relativedelta import relativedelta @@ -215,3 +216,35 @@ class Duration(Converter): now = datetime.utcnow() return now + delta + + +class ISODateTime(Converter): + """"Converts an ISO-8601 datetime string into a datetime.datetime.""" + + async def convert(self, ctx: Context, datetime_string: str) -> datetime: + """ + Converts a ISO-8601 `datetime_string` into a `datetime.datetime` object. + + The converter is flexible in the formats it accepts, as it uses the `isoparse` method of + `dateutil.parser`. In general, it accepts datetime strings that start with a date, + optionally followed by a time. + + See: + + Formats that are guaranteed to be valid by our tests are: + + - `YYYY-mm-ddTHH:MM:SS` | `YYYY-mm-dd HH:MM:SS` + - `YYYY-mm-ddTHH:MM` | `YYYY-mm-dd HH:MM` + - `YYYY-mm-dd` + - `YYYY-mm` + - `YYYY` + + Note: ISO-8601 specifies a `T` as the separator between the date and the time part of the + datetime string. The converter accepts both a `T` and a single space character. + """ + try: + dt = dateutil.parser.isoparse(datetime_string) + except ValueError: + raise BadArgument(f"`{datetime_string}` is not a valid ISO-8601 datetime string") + + return dt diff --git a/tests/test_converters.py b/tests/test_converters.py index 35fc5d88e..aa692f9f8 100644 --- a/tests/test_converters.py +++ b/tests/test_converters.py @@ -8,6 +8,7 @@ from discord.ext.commands import BadArgument from bot.converters import ( Duration, + ISODateTime, TagContentConverter, TagNameConverter, ValidPythonIdentifier, @@ -184,3 +185,57 @@ def test_duration_converter_for_invalid(duration: str): converter = Duration() with pytest.raises(BadArgument, match=f'`{duration}` is not a valid duration string.'): asyncio.run(converter.convert(None, duration)) + + +@pytest.mark.parametrize( + ("datetime_string", "expected_dt"), + ( + # `YYYY-mm-ddTHH:MM:SS` | `YYYY-mm-dd HH:MM:SS` + ('2019-09-02T02:03:05', datetime.datetime(2019, 9, 2, 2, 3, 5)), + ('2019-09-02 02:03:05', datetime.datetime(2019, 9, 2, 2, 3, 5)), + + # `YYYY-mm-ddTHH:MM` | `YYYY-mm-dd HH:MM` + ('2019-11-12T09:15', datetime.datetime(2019, 11, 12, 9, 15)), + ('2019-11-12 09:15', datetime.datetime(2019, 11, 12, 9, 15)), + + # `YYYY-mm-dd` + ('2019-04-01', datetime.datetime(2019, 4, 1)), + + # `YYYY-mm` + ('2019-02-01', datetime.datetime(2019, 2, 1)), + + # `YYYY` + ('2025', datetime.datetime(2025, 1, 1)), + ), +) +def test_isodatetime_converter_for_valid(datetime_string: str, expected_dt: datetime.datetime): + converter = ISODateTime() + assert asyncio.run(converter.convert(None, datetime_string)) == expected_dt + + +@pytest.mark.parametrize( + ("datetime_string"), + ( + # Make sure it doesn't interfere with the Duration converation + ('1Y'), + ('1d'), + ('1H'), + + # Check if it fails when only providing the optional time part + ('10:10:10'), + ('10:00'), + + # Invalid date format + ('19-01-01'), + + # Other non-valid strings + ('fisk the tag master'), + ), +) +def test_isodatetime_converter_for_invalid(datetime_string: str): + converter = ISODateTime() + with pytest.raises( + BadArgument, + match=f"`{datetime_string}` is not a valid ISO-8601 datetime string", + ): + asyncio.run(converter.convert(None, datetime_string)) -- cgit v1.2.3 From 5f81b80a4dea49195053ab0177f4fd9aa9bea5e5 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com> Date: Wed, 2 Oct 2019 07:27:06 +0200 Subject: Apply docstring review suggestion Co-Authored-By: Mark --- tests/test_converters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests/test_converters.py') diff --git a/tests/test_converters.py b/tests/test_converters.py index aa692f9f8..8093f55ac 100644 --- a/tests/test_converters.py +++ b/tests/test_converters.py @@ -216,7 +216,7 @@ def test_isodatetime_converter_for_valid(datetime_string: str, expected_dt: date @pytest.mark.parametrize( ("datetime_string"), ( - # Make sure it doesn't interfere with the Duration converation + # Make sure it doesn't interfere with the Duration converter ('1Y'), ('1d'), ('1H'), -- cgit v1.2.3 From a8b600217cb9ab4524bb307f0a6a922a0d8815be Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com> Date: Wed, 2 Oct 2019 10:42:43 +0200 Subject: Make ISODateTime return tz-unaware datetime 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. --- bot/converters.py | 13 ++++++++++++- tests/test_converters.py | 21 +++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) (limited to 'tests/test_converters.py') diff --git a/bot/converters.py b/bot/converters.py index 59a6f6b07..27223e632 100644 --- a/bot/converters.py +++ b/bot/converters.py @@ -5,6 +5,7 @@ from ssl import CertificateError from typing import Union import dateutil.parser +import dateutil.tz import discord from aiohttp import ClientConnectorError from dateutil.relativedelta import relativedelta @@ -227,12 +228,18 @@ class ISODateTime(Converter): The converter is flexible in the formats it accepts, as it uses the `isoparse` method of `dateutil.parser`. In general, it accepts datetime strings that start with a date, - optionally followed by a time. + optionally followed by a time. Specifying a timezone offset in the datetime string is + supported, but the `datetime` object will be converted to UTC and will be returned without + `tzinfo` as a timezone-unaware `datetime` object. See: https://dateutil.readthedocs.io/en/stable/parser.html#dateutil.parser.isoparse Formats that are guaranteed to be valid by our tests are: + - `YYYY-mm-ddTHH:MM:SSZ` | `YYYY-mm-dd HH:MM:SSZ` + - `YYYY-mm-ddTHH:MM:SS±HH:MM` | `YYYY-mm-dd HH:MM:SS±HH:MM` + - `YYYY-mm-ddTHH:MM:SS±HHMM` | `YYYY-mm-dd HH:MM:SS±HHMM` + - `YYYY-mm-ddTHH:MM:SS±HH` | `YYYY-mm-dd HH:MM:SS±HH` - `YYYY-mm-ddTHH:MM:SS` | `YYYY-mm-dd HH:MM:SS` - `YYYY-mm-ddTHH:MM` | `YYYY-mm-dd HH:MM` - `YYYY-mm-dd` @@ -247,4 +254,8 @@ class ISODateTime(Converter): except ValueError: raise BadArgument(f"`{datetime_string}` is not a valid ISO-8601 datetime string") + if dt.tzinfo: + dt = dt.astimezone(dateutil.tz.UTC) + dt = dt.replace(tzinfo=None) + return dt diff --git a/tests/test_converters.py b/tests/test_converters.py index 8093f55ac..86e8f2249 100644 --- a/tests/test_converters.py +++ b/tests/test_converters.py @@ -190,6 +190,27 @@ def test_duration_converter_for_invalid(duration: str): @pytest.mark.parametrize( ("datetime_string", "expected_dt"), ( + + # `YYYY-mm-ddTHH:MM:SSZ` | `YYYY-mm-dd HH:MM:SSZ` + ('2019-09-02T02:03:05Z', datetime.datetime(2019, 9, 2, 2, 3, 5)), + ('2019-09-02 02:03:05Z', datetime.datetime(2019, 9, 2, 2, 3, 5)), + + # `YYYY-mm-ddTHH:MM:SS±HH:MM` | `YYYY-mm-dd HH:MM:SS±HH:MM` + ('2019-09-02T03:18:05+01:15', datetime.datetime(2019, 9, 2, 2, 3, 5)), + ('2019-09-02 03:18:05+01:15', datetime.datetime(2019, 9, 2, 2, 3, 5)), + ('2019-09-02T00:48:05-01:15', datetime.datetime(2019, 9, 2, 2, 3, 5)), + ('2019-09-02 00:48:05-01:15', datetime.datetime(2019, 9, 2, 2, 3, 5)), + + # `YYYY-mm-ddTHH:MM:SS±HHMM` | `YYYY-mm-dd HH:MM:SS±HHMM` + ('2019-09-02T03:18:05+0115', datetime.datetime(2019, 9, 2, 2, 3, 5)), + ('2019-09-02 03:18:05+0115', datetime.datetime(2019, 9, 2, 2, 3, 5)), + ('2019-09-02T00:48:05-0115', datetime.datetime(2019, 9, 2, 2, 3, 5)), + ('2019-09-02 00:48:05-0115', datetime.datetime(2019, 9, 2, 2, 3, 5)), + + # `YYYY-mm-ddTHH:MM:SS±HH` | `YYYY-mm-dd HH:MM:SS±HH` + ('2019-09-02 03:03:05+01', datetime.datetime(2019, 9, 2, 2, 3, 5)), + ('2019-09-02T01:03:05-01', datetime.datetime(2019, 9, 2, 2, 3, 5)), + # `YYYY-mm-ddTHH:MM:SS` | `YYYY-mm-dd HH:MM:SS` ('2019-09-02T02:03:05', datetime.datetime(2019, 9, 2, 2, 3, 5)), ('2019-09-02 02:03:05', datetime.datetime(2019, 9, 2, 2, 3, 5)), -- cgit v1.2.3 From 629cb4d05405a155715da765a2408be9156eb215 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com> Date: Thu, 3 Oct 2019 07:14:31 +0200 Subject: Check if tzinfo is None in ISODateTime test 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. --- tests/test_converters.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'tests/test_converters.py') diff --git a/tests/test_converters.py b/tests/test_converters.py index 86e8f2249..f69995ec6 100644 --- a/tests/test_converters.py +++ b/tests/test_converters.py @@ -231,7 +231,9 @@ def test_duration_converter_for_invalid(duration: str): ) def test_isodatetime_converter_for_valid(datetime_string: str, expected_dt: datetime.datetime): converter = ISODateTime() - assert asyncio.run(converter.convert(None, datetime_string)) == expected_dt + converted_dt = asyncio.run(converter.convert(None, datetime_string)) + assert converted_dt.tzinfo is None + assert converted_dt == expected_dt @pytest.mark.parametrize( -- cgit v1.2.3