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

Change hijri calendar dependency to support Python 3 #718

Merged
merged 10 commits into from
Jul 15, 2020

Conversation

noviluni
Copy link
Collaborator

@noviluni noviluni commented Jun 23, 2020

The current package we are using to parse dates from the Hijri calendar (https://pypi.org/project/ummalqura/) is not maintained and it doesn't support Python 3.

I think that we can use this other package, as it has been maintained and it works pretty well, containing all the features we need: https://github.com/dralshehri/hijri-converter

You can see in the diff below how simple it is to adapt this new package to the current dateparser status (tests are passing).

This library only supports Python 3.6+, but that's not a problem, as we will remove Python 2.7 support in the next release and the end of life of Python 3.5 is 2020-09-13 (the next release of dateparser is planned for September or October, we don't need to remove support for python 3.5 but just add a note about the hijri calendar compatibility).

fixes: #385

@noviluni noviluni added this to the v1.0.0 milestone Jun 23, 2020
@codecov
Copy link

codecov bot commented Jun 23, 2020

Codecov Report

Merging #718 into master will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #718      +/-   ##
==========================================
+ Coverage   95.49%   95.61%   +0.12%     
==========================================
  Files         304      304              
  Lines        2662     2646      -16     
==========================================
- Hits         2542     2530      -12     
+ Misses        120      116       -4     
Impacted Files Coverage Δ
dateparser/calendars/hijri_parser.py 89.74% <100.00%> (+2.24%) ⬆️
dateparser/parser.py 99.02% <0.00%> (+<0.01%) ⬆️
dateparser/date.py 99.57% <0.00%> (+0.80%) ⬆️

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 b256f14...97b7335. Read the comment docs.

tests/test_hijri.py Outdated Show resolved Hide resolved
tests/test_hijri.py Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

Looks good to me.

On a separate note, given the next release will remove Python 2 support, is this really a breaking change?

@noviluni
Copy link
Collaborator Author

noviluni commented Jun 29, 2020

Oh, it wouldn't be a breaking change.

I am waiting 2 weeks to ensure that the last version is stable enough before proceeding with merging those PRs that would break the Python 2.7 code and marked them with the "breaking change" label to be easily recognizable. I just wrote about this here: #727 (comment)

I agree with you that in the next release notes this shouldn't be labeled as a "breaking change".

@noviluni
Copy link
Collaborator Author

noviluni commented Jul 3, 2020

code rebased against the master branch to fix conflicts

@noviluni noviluni merged commit 1b546ad into scrapinghub:master Jul 15, 2020
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.

ummalqurra is supported on Python 3
2 participants