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') 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') 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