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

Deprecation removals for Scrapy 1.7 #3578

Merged
merged 2 commits into from Jul 8, 2019
Merged

Deprecation removals for Scrapy 1.7 #3578

merged 2 commits into from Jul 8, 2019

Conversation

@nyov
Copy link
Contributor

@nyov nyov commented Jan 12, 2019

Removing deprecations of 2015 and prior (pre-1.1).

@nyov
Copy link
Contributor Author

@nyov nyov commented Jan 12, 2019

Damn hub client didn't take my text :( So here goes:

Here is a list of current deprecations that have been around since before 2016, in some form, according to git blame (Scrapy 1.1 and before); some even longer, dating back to 2010.

I am not sure how long it makes sense to keep them around, certainly legacy code benefits from the ability to continue to run on newer Scrapy versions unchanged,
but I also feel that it might prevent the codebase from further evolving after a certain point, as it becomes a hassle to track the deprecated APIs.
(Can't see further possible improvements with all the half-dead code around ;)

Let me know what you think about these proposals, I was too lazy to do feature-(un)commits for this, but it's certainly not all-or-nothing.

@kmike kmike added this to the v1.7 milestone Jan 30, 2019
scrapy/selector/unified.py Outdated Show resolved Hide resolved
@kmike
Copy link
Member

@kmike kmike commented Feb 15, 2019

+1 to deprecate this all for 1.7 release.

@nyov
Copy link
Contributor Author

@nyov nyov commented Feb 17, 2019

.x is a shorthand on SelectorList, which I removed and then decided not to. I must have mistakenly re-added it to Selector as well. Would you say that I should remove it, or is it a convenient one-letter shorthand to keep?

@kmike
Copy link
Member

@kmike kmike commented Feb 18, 2019

@nyov I think we should remove .x, it was deprecated ~10 years ago: 48b40bd#diff-15415c3ac7385abcfb0fb020668d7ec5

@nyov nyov force-pushed the deprecations branch from 456f63e to 55debc9 Feb 18, 2019
@codecov
Copy link

@codecov codecov bot commented Feb 18, 2019

Codecov Report

Merging #3578 into master will decrease coverage by 0.5%.
The diff coverage is 75%.

@@            Coverage Diff            @@
##           master   #3578      +/-   ##
=========================================
- Coverage    84.5%     84%   -0.51%     
=========================================
  Files         167     162       -5     
  Lines        9406    9241     -165     
  Branches     1397    1382      -15     
=========================================
- Hits         7949    7763     -186     
- Misses       1199    1220      +21     
  Partials      258     258
Impacted Files Coverage Δ
scrapy/loader/__init__.py 95.03% <ø> (+0.51%) ⬆️
scrapy/utils/python.py 83.23% <ø> (+0.55%) ⬆️
scrapy/cmdline.py 68% <ø> (+0.59%) ⬆️
scrapy/spiders/__init__.py 98.27% <ø> (-0.3%) ⬇️
scrapy/utils/response.py 88.63% <ø> (+3.21%) ⬆️
scrapy/core/downloader/handlers/http.py 100% <ø> (ø) ⬆️
scrapy/spiders/crawl.py 77.46% <ø> (-0.92%) ⬇️
scrapy/selector/__init__.py 100% <ø> (ø) ⬆️
scrapy/core/downloader/__init__.py 90.97% <ø> (+2%) ⬆️
scrapy/extensions/feedexport.py 70% <0%> (-8.47%) ⬇️
... and 14 more

@codecov
Copy link

@codecov codecov bot commented Feb 18, 2019

Codecov Report

Merging #3578 into master will decrease coverage by 0.71%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3578      +/-   ##
==========================================
- Coverage   85.42%   84.71%   -0.72%     
==========================================
  Files         169      162       -7     
  Lines        9701     9242     -459     
  Branches     1445     1382      -63     
==========================================
- Hits         8287     7829     -458     
+ Misses       1166     1159       -7     
- Partials      248      254       +6
Impacted Files Coverage Δ
scrapy/loader/__init__.py 95.03% <ø> (+0.16%) ⬆️
scrapy/utils/python.py 83.23% <ø> (-0.45%) ⬇️
scrapy/cmdline.py 68% <ø> (+0.59%) ⬆️
scrapy/spiders/__init__.py 98.27% <ø> (-0.3%) ⬇️
scrapy/utils/response.py 88.63% <ø> (+3.21%) ⬆️
scrapy/core/downloader/handlers/http.py 100% <ø> (ø) ⬆️
scrapy/spiders/crawl.py 77.46% <ø> (-4.89%) ⬇️
scrapy/selector/__init__.py 100% <ø> (ø) ⬆️
scrapy/core/downloader/__init__.py 90.97% <ø> (+1.92%) ⬆️
scrapy/pipelines/files.py 66.53% <100%> (-1.71%) ⬇️
... and 42 more

@nyov nyov changed the title Deprecation removals for Scrapy 1.6 Deprecation removals for Scrapy 1.7 Feb 18, 2019
@nyov
Copy link
Contributor Author

@nyov nyov commented Feb 18, 2019

@kmike, done.

I have one further undecided change, regarding S3FeedStorage:

For the intended 1.6 target, I changed the from scrapy.conf import settings line into
from scrapy.utils.project import get_project_settings, though perhaps for 1.7 it is possible to remove the whole codeblock?

if no_defaults:
from scrapy.utils.project import get_project_settings
settings = get_project_settings()
if 'AWS_ACCESS_KEY_ID' in settings or 'AWS_SECRET_ACCESS_KEY' in settings:
import warnings
from scrapy.exceptions import ScrapyDeprecationWarning
warnings.warn(
"Initialising `scrapy.extensions.feedexport.S3FeedStorage` "
"without AWS keys is deprecated. Please supply credentials or "
"use the `from_crawler()` constructor.",
category=ScrapyDeprecationWarning,
stacklevel=2
)
access_key = settings['AWS_ACCESS_KEY_ID']
secret_key = settings['AWS_SECRET_ACCESS_KEY']

(It was added in 4d77c30.)

That would be useful to me since I don't know how to change the test to mock.patch get_project_settings in tests/test_feedexport.py:

-    @mock.patch('scrapy.conf.settings', new={'AWS_ACCESS_KEY_ID': 'conf_key',
-                'AWS_SECRET_ACCESS_KEY': 'conf_secret'}, create=True)
[...]
-        # Backwards compatibility for initialising without settings
-        with warnings.catch_warnings(record=True) as w:
-            storage = S3FeedStorage('s3://mybucket/export.csv')
-            self.assertEqual(storage.access_key, 'conf_key')
-            self.assertEqual(storage.secret_key, 'conf_secret')
-            self.assertTrue('without AWS keys' in str(w[-1].message))

Then I could just drop the test. :)

@kmike
Copy link
Member

@kmike kmike commented Feb 19, 2019

Thanks @nyov!

I'm not sure we should drop S3FeedStorage backwards compatibility code yet, because it was added in 1.6 (see #3348).

@nyov
Copy link
Contributor Author

@nyov nyov commented Feb 23, 2019

Ah, okay. I was only checking the blame dates, my bad.
But I don't know how to fix the testcase, sorry. (Strangely it only fails on Py2.7/jessie tox env, after removing the mock.patch line, perhaps it's broken anyhow?)

edit: I fixed the testcase, hopefully correctly.

@nyov nyov force-pushed the deprecations branch 3 times, most recently from 8be4ae3 to 90f1bcb Feb 24, 2019
@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Jul 4, 2019

@nyov Could you please fix the conflicts introduced by other commits?

@nyov nyov force-pushed the deprecations branch from 90f1bcb to 5442c2d Jul 6, 2019
@nyov
Copy link
Contributor Author

@nyov nyov commented Jul 6, 2019

Ok, done.
Build looks ok,
Those conflicts were only changes to code which is removed by this commit anyway.

@kmike
Copy link
Member

@kmike kmike commented Jul 8, 2019

Thanks @nyov!

@kmike kmike merged commit 4bef6f4 into scrapy:master Jul 8, 2019
2 checks passed
@nyov
Copy link
Contributor Author

@nyov nyov commented Jul 8, 2019

And, as always, thank you guys for reviewing.

@nyov nyov deleted the deprecations branch Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants