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 deprecated Crawler.spiders property #4398

Merged
merged 1 commit into from Mar 16, 2020

Conversation

nyov
Copy link
Contributor

@nyov nyov commented Mar 3, 2020

Two commits,
the 1st removing Crawler.spiders; deprecated since 4190266 (2014, Scrapy 0.25)
the 2nd removing CrawlerRunner.spiders; deprecated since ad587ea (2015, Scrapy 0.25.1)

Adding SPIDER_MANAGER_CLASS to scrapy/settings/deprecated.py is somewhat too late, just for completeness' sake; so it's been there and can be removed again next commit?

@nyov nyov force-pushed the remove-crawler-spiders branch from 814e353 to 1140a05 Compare Mar 3, 2020
@codecov
Copy link

@codecov codecov bot commented Mar 3, 2020

Codecov Report

Merging #4398 into master will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4398      +/-   ##
==========================================
+ Coverage   84.51%   84.56%   +0.04%     
==========================================
  Files         166      166              
  Lines        9869     9864       -5     
  Branches     1467     1466       -1     
==========================================
  Hits         8341     8341              
+ Misses       1270     1268       -2     
+ Partials      258      255       -3     
Impacted Files Coverage Δ
scrapy/utils/misc.py 97.11% <0.00%> (+1.92%) ⬆️
scrapy/utils/defer.py 95.65% <0.00%> (+2.17%) ⬆️
scrapy/utils/trackref.py 85.71% <0.00%> (+2.85%) ⬆️

@kmike
Copy link
Member

@kmike kmike commented Mar 4, 2020

Hey @nyov! Are spider loader cleanups related to this issue? If no, how hard would it be to separate those into a different PR?

@nyov
Copy link
Contributor Author

@nyov nyov commented Mar 4, 2020

Sure, no problem.
But do you mean the move of the _get_spider_loader function into a static method of CrawlerRunner?
Or the removal of the unused SpiderLoader class from the testcase?

@nyov nyov force-pushed the remove-crawler-spiders branch from 507df88 to c481b81 Compare Mar 4, 2020
@nyov
Copy link
Contributor Author

@nyov nyov commented Mar 4, 2020

Ok I'll just put the second commit as another pull request.
Rebased.

Deprecated since 4190266 (2014, Scrapy 0.25)
@nyov nyov force-pushed the remove-crawler-spiders branch from c481b81 to b1566a6 Compare Mar 4, 2020
@kmike
Copy link
Member

@kmike kmike commented Mar 13, 2020

Hey @nyov! I meant the move of the _get_spider_loader function into a static method of CrawlerRunner. Is it needed?

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Mar 13, 2020

It is certainly not necessary.

However, after the change I think the readability is slightly better: it is now obvious that CrawlerRunner is the only user of that function.

That said, if we expect the function to have more users in the long run, I guess it would be better to leave it as it was.

@kmike
Copy link
Member

@kmike kmike commented Mar 13, 2020

I was making this comment more from a process perspective: merging a change which removes a deprecated "spider" property is a no-brainer, but if a PR also introducs such cleanup, it slows down the process. For example,

  • should this function be a classmethod or a staticmethod,
  • are there users of this function which we'll break after a move (and if so, should we do a deprecation);
  • some time is needed to check if it is a copy-paste, or something changed in the implementation
  • some time is needed to figure out if it is actually better to have it as a function or as a method

It may turn up that in the end it is a good change to make, but hope this makes it more clear why I asked if a PR can be split.

wRAR
wRAR approved these changes Mar 16, 2020
@wRAR wRAR merged commit ecd3a97 into scrapy:master Mar 16, 2020
2 checks passed
@nyov
Copy link
Contributor Author

@nyov nyov commented Mar 17, 2020

Sorry I wasn't fast enough with a reply. GH failed on me, and now you took it out of my hands :)

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.

None yet

4 participants