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

Incorrect date when using .when with weekdays #146

Closed
alxwrd opened this issue Apr 15, 2018 · 7 comments
Closed

Incorrect date when using .when with weekdays #146

alxwrd opened this issue Apr 15, 2018 · 7 comments
Assignees

Comments

@alxwrd
Copy link
Contributor

alxwrd commented Apr 15, 2018

hello! 👋

There appears to be an issue with maya.when() when using weekday names.

>>> today = maya.now()
>>> print(today)
'Sun, 15 Apr 2018 16:58:03 GMT'

>>> tomorrow = maya.when("monday", prefer_past=False)
>>> print(tomorrow)
'Mon, 09 Apr 2018 00:00:00 GMT'

dateparser seems to handle this correctly, but only when the setting 'PREFER_DATES_FROM' is set to 'future'.

>>> dateparser.parse("monday")
datetime.datetime(2018, 4, 9, 0, 0)

>>> dateparser.parse("monday", settings={"PREFER_DATES_FROM": "future"})
datetime.datetime(2018, 4, 16, 0, 0)

>>> dateparser.parse("Apr 16")
datetime.datetime(2018, 4, 16, 0, 0)

>>> dateparser.parse("Apr 16", settings={"PREFER_DATES_FROM": "past"})
datetime.datetime(2017, 4, 16, 0, 0)

The default setting for dateparser appears to be "current_period" from dateparser/dateparser_data/settings.py.

As the only way to alter the 'PREFER_DATES_FROM' setting is change prefer_past, there is currently no way to get the future day in this instance.

I'll open a PR with a failing test.

@timofurrer
Copy link
Collaborator

Hi! Thanks for contribution!

I'll open a PR with a failing test.

Great! 🎉

@kennethreitz what about exposing the PREFER_DATES_FROM setting from dateparser via the maya.when() API?

Something like:

maya.when('monday', prefer_dates_from='past')

@alxwrd
Copy link
Contributor Author

alxwrd commented Apr 15, 2018

@timofurrer I had thought:

-def when(string, timezone='UTC', prefer_past=False):
+def when(string, timezone='UTC', prefer_past=None):

-    if prefer_past:
+    if prefer_past is False:
+        settings['PREFER_DATES_FROM'] = 'future'
+    if prefer_past is True:
         settings['PREFER_DATES_FROM'] = 'past'

     dt = dateparser.parse(string, settings=settings)

This should allow all three of dateparsers options of 'current_period', 'future', and 'past' and not break anything else.

@timofurrer
Copy link
Collaborator

Sounds even better!
I'm looking forward to accept the change in the PR you've already made ;)

@timofurrer
Copy link
Collaborator

timofurrer commented Apr 16, 2018

I might have second thoughts about the proposed API.
The thing is, that for me, setting prefer_past=False doesn't intuitively mean that it should prefer the future. It would just mean that not the past should be preferred and therefore the current_period is used. Which isn't the case.

Having both arguments prefer_past and prefer_future available leads to inconsistencies and therefore is not really an option. Which leads us to the prefer_dates_from argument. I know it's kinda verbose but the best I can come up with right now. And thoughts?

@alxwrd
Copy link
Contributor Author

alxwrd commented Apr 16, 2018

I agree.

My thoughts behind keeping prefer_past was that it's a non breaking change.

I think because prefer_dates_from is more verbose, it might be nicer.

-def when(string, timezone='UTC', prefer_past=False):
+def when(string, timezone='UTC', prefer_dates_from='current_period'):

    settings = {
         'TIMEZONE': timezone,
         'RETURN_AS_TIMEZONE_AWARE': True,
         'TO_TIMEZONE': 'UTC',
+        'PREFER_DATES_FROM': prefer_dates_from,
     }
-    if prefer_past:
-        settings['PREFER_DATES_FROM'] = 'past'
+
     dt = dateparser.parse(string, settings=settings)
     if dt is None:
         raise ValueError('invalid datetime input specified.')

Let me know and I can push up some changes ✨

@timofurrer
Copy link
Collaborator

Yes, let's do that! @kennethreitz any thoughts on that one?

@timofurrer
Copy link
Collaborator

Done in #147

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants