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

[MRG+1] TST make it clear which requirements are Python 2-only #3311

Merged
merged 1 commit into from Jul 3, 2018

Conversation

@kmike
Copy link
Member

@kmike kmike commented Jun 29, 2018

I was always forgetting if requirements-py3.txt is applied on top of requirements.txt (false), or if they are totally separate requirements files (true). To avoid confusion, requirements.txt is renamed to requirements-py2.txt in this PR.

  • rename requirements.txt to requirements-py2.txt, to make it clear they are Python 2-only
  • make requirements-py3.txt consistent with requirements-py2.txt
* rename requirements.txt to requirements-py2.txt, to make it clear they are Python 2-only
* make requirements-py3.txt consistent with requirements-py2.txt
@kmike kmike requested a review from dangra Jun 29, 2018
@codecov
Copy link

@codecov codecov bot commented Jun 29, 2018

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3311      +/-   ##
==========================================
- Coverage   82.14%   82.13%   -0.02%     
==========================================
  Files         228      228              
  Lines        9605     9605              
  Branches     1387     1387              
==========================================
- Hits         7890     7889       -1     
  Misses       1458     1458              
- Partials      257      258       +1
Impacted Files Coverage Δ
scrapy/utils/trackref.py 83.78% <0%> (-2.71%) ⬇️
@lopuhin lopuhin changed the title TST make it clear which requirements are Python 2-only [MRG+1] TST make it clear which requirements are Python 2-only Jun 29, 2018
Copy link
Member

@lopuhin lopuhin left a comment

requirements-py2.txt makes much more sense, 👍

@grammy-jiang
Copy link
Contributor

@grammy-jiang grammy-jiang commented Jun 30, 2018

BTW, since Python 2.7 will not be supported after 2020 officially, is there any schedule for Scrapy to give up Python 2.7 support?

@dangra dangra merged commit 8d93691 into master Jul 3, 2018
2 checks passed
2 checks passed
codecov/patch Coverage not affected when comparing 64f48ef...f11d65f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dangra dangra deleted the rename-requirements branch Jul 3, 2018
@kmike
Copy link
Member Author

@kmike kmike commented Jul 3, 2018

@grammy-jiang no schedule yet. I don't think we'll be too aggressive.

@grammy-jiang
Copy link
Contributor

@grammy-jiang grammy-jiang commented Jul 4, 2018

@kmike

Ok, now I have to import a third party package in order to compatible with Python 2.7. Is there any specific guideline about importing the new package in this project? I have read the page of contribution, and find nothing related.

The package is functools32, which is developed by the author of the module functools, in order to provide the missing methods in Python 2.7 comparing with Python 3.x, because Python 2.7 has stopped to adding new features only fix bugs.

@kmike
Copy link
Member Author

@kmike kmike commented Jul 4, 2018

Hey @grammy-jiang! functools32 is now a parsel dependency (since yesterday), and parsel is a Scrapy dependency, so you can count on it being available. But it is better to add this dependency explicitly to Scrapy as well if you're using it there, so that Scrapy doesn't rely on transitive dependencies; check how it is done in parsel's setup.py.

@grammy-jiang
Copy link
Contributor

@grammy-jiang grammy-jiang commented Jul 4, 2018

Hi, @kmike

Thanks for your reply.

I see, in parsel, six.PY2 is used to choose where the method lru_cache is imported from. That's impressive.

But why not use try and except to do this, just like what we did here?

@kmike
Copy link
Member Author

@kmike kmike commented Jul 4, 2018

But why not use try and except to do this, just like what we did here?

I don't think it matters much, either way is fine.

@grammy-jiang
Copy link
Contributor

@grammy-jiang grammy-jiang commented Jul 4, 2018

That's great, thanks!

@kmike kmike added this to the v1.6 milestone Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants