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

apply PREFER_DAY_OF_MONTH in date.py #611

Merged
merged 2 commits into from
Mar 6, 2020

Conversation

noviluni
Copy link
Collaborator

fixes #607

@codecov
Copy link

codecov bot commented Jan 17, 2020

Codecov Report

Merging #611 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #611      +/-   ##
=========================================
+ Coverage   95.12%   95.2%   +0.08%     
=========================================
  Files         302     302              
  Lines        2503    2505       +2     
=========================================
+ Hits         2381    2385       +4     
+ Misses        122     120       -2
Impacted Files Coverage Δ
dateparser/parser.py 98.88% <100%> (+0.53%) ⬆️
dateparser/utils/__init__.py 96.8% <100%> (+0.27%) ⬆️
dateparser/date.py 97.94% <100%> (-0.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4b493f...421ae78. Read the comment docs.

@noviluni
Copy link
Collaborator Author

noviluni commented Jan 17, 2020

(Explanation and example in a comment in the linked issue)

@Gallaecio
Copy link
Member

What I found most strange about #607 was that, depending on whether the date format input parameter was a list (['%m/%Y']) or a string ('%m/%Y'), the output date was different.

I think that is something we need to address and cover in the tests before we can consider #607 solved.

@noviluni
Copy link
Collaborator Author

noviluni commented Jan 17, 2020

@Gallaecio
I think there was a confusion.

Currently, doing this:

>>> dateparser.parse('02/2013', ['%m/%Y'])                                                                                                                                                                  
datetime.datetime(2013, 2, 28, 0, 0)

Is the same than doing this:

>>> dateparser.parse('02/2013', '%m/%Y')                                                                                                                                                                    
/home/marc/workprojects/dateparser/dateparser/date.py:142: FutureWarning: Date formats should be list, tuple or set of strings
  warn(_DateLocaleParser.DATE_FORMATS_ERROR_MESSAGE, FutureWarning)
datetime.datetime(2013, 2, 28, 0, 0)

(except for the deprecation warning). In the example in the issue, the second statement is

datetime.strptime('02/2013', '%m/%Y')

which is using the datetime.strptime, not the dateparse.parse method.

@Gallaecio
Copy link
Member

🤦‍♂️

return calendar.monthrange(year, month)[1]


def set_correct_day_from_settings(date_obj, settings, last_day=None, current_day=None):
Copy link
Member

Choose a reason for hiding this comment

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

💄 What about apply_day_of_the_month_from_settings, to be consistent with apply_timezone_from_settings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm... This was my first approach, but then I didn't like that naming. First of all "apply" seems to mean that you are "adding" something. It maybe has sense for the timezone, as sometimes you add it, but I'm not sure if this has sense for the day as what you are doing is to change it.

Secondly, apply_day_of_the_month_from_settings seems to mean that it is getting the day from the settings; but what it is doing is using the rules in the settings to find the correct day.

For both reasons I prefer this naming: set_correct_day_from_settings.

What do you think @Gallaecio ? Any better idea?

dateparser/parser.py Outdated Show resolved Hide resolved
dateparser/utils/__init__.py Show resolved Hide resolved
@noviluni
Copy link
Collaborator Author

Note to self: I could also add a unit test for the function get_last_day_of_month (including all months and at least one example of leap year).

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.

dateparser.parse('02/2013', ['%m/%Y']) gives wrong datetime
2 participants