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

Remove pypy and python2 from tox and travis. #744

Merged
merged 1 commit into from
Jul 16, 2020

Conversation

kishan3
Copy link

@kishan3 kishan3 commented Jul 15, 2020

This should help one step in closing #727

This should help one step in closing scrapinghub#727
@codecov
Copy link

codecov bot commented Jul 15, 2020

Codecov Report

Merging #744 into master will decrease coverage by 0.37%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #744      +/-   ##
==========================================
- Coverage   95.61%   95.23%   -0.38%     
==========================================
  Files         304      304              
  Lines        2646     2646              
==========================================
- Hits         2530     2520      -10     
- Misses        116      126      +10     
Impacted Files Coverage Δ
dateparser/utils/strptime.py 92.68% <0.00%> (-7.32%) ⬇️
dateparser/utils/__init__.py 94.81% <0.00%> (-2.23%) ⬇️
dateparser/search/search.py 98.06% <0.00%> (-1.30%) ⬇️
dateparser/date.py 99.15% <0.00%> (-0.43%) ⬇️
dateparser/languages/locale.py 98.39% <0.00%> (-0.27%) ⬇️

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 1b546ad...a919398. Read the comment docs.

@Gallaecio
Copy link
Member

The coverage data reveals a few parts of the code that are specific to Python 2 and can be removed.

Copy link
Collaborator

@noviluni noviluni left a comment

Choose a reason for hiding this comment

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

The coverage data reveals a few parts of the code that are specific to Python 2 and can be removed.

@Gallaecio , let's do this in a next PR!

@lopuhin
Copy link
Member

lopuhin commented Jul 15, 2020

re pypy removal - maybe it makes sense to add pypy3 then, maybe as part of another PR?

@noviluni
Copy link
Collaborator

Hi @lopuhin, I tried it and the tests were totally broken. The regex package doesn't have official support for pypy and pypy3. I would love to have support for pypy3, but I think it's not as easy as it seems (maybe it's not possible) and it should be added in another, different, PR. Thank you for your comment, I opened this new ticket to discuss it: #745

@noviluni
Copy link
Collaborator

noviluni commented Jul 16, 2020

Anyway, this is the first step on a list of steps to get rid of Python 2.

We will:

  • Remove Python 2 testing (this will make it easier to create new PRs without needing to support Python 2.7)
  • Update setup file
  • Migrate code (probably using 2to3 and checking it carefully)
  • Delete other reminiscences (the six package, version-specific code...)

We don't need to perform these steps in this strict order, but I think it's a good way to go, let me know if there is any wrong on this approach or if you have any other idea.

@Gallaecio @lopuhin

@lopuhin
Copy link
Member

lopuhin commented Jul 16, 2020

Thanks @noviluni yes makes sense to tackle pypy3 separately. Re the steps you outlined - they make sense, I don't have experience with 2to3, only with manual porting, but usually py2-specific code stands out.

@kishan3
Copy link
Author

kishan3 commented Jul 16, 2020

Hi @noviluni @Gallaecio we should merge this only after coverage is fixed or can do now?

@Gallaecio
Copy link
Member

Hi @noviluni @Gallaecio we should merge this only after coverage is fixed or can do now?

I think we can merge already and tackle the coverage part separately, as @noviluni suggested.

@noviluni noviluni merged commit cf0673c into scrapinghub:master Jul 16, 2020
@noviluni
Copy link
Collaborator

Good job! Now we can say that we don't support Python 3 officially 🤣

This was referenced Jul 17, 2020
@noviluni noviluni added this to the v1.0.0 milestone Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants