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] Check that spidercls arguments in scrapy.crawler classes are not spid… #3610

Merged
merged 1 commit into from Mar 15, 2019

Conversation

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Feb 1, 2019

…er objects

I think this is a better approach to fix #2283. Instead of trying to make the documentation more obvious, have the code fail gracefully with a useful error message.

@Gallaecio
Copy link
Member Author

@Gallaecio Gallaecio commented Feb 1, 2019

I’ll review the failing test next week, it probably needs updated expectations.

@kmike
Copy link
Member

@kmike kmike commented Feb 5, 2019

+1 to make this change after fixing tests. It seems that the change allowed to find a bug in Scrapy testing suite.

@codecov
Copy link

@codecov codecov bot commented Feb 11, 2019

Codecov Report

Merging #3610 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3610      +/-   ##
==========================================
+ Coverage    84.5%   84.53%   +0.02%     
==========================================
  Files         167      167              
  Lines        9406     9413       +7     
  Branches     1397     1400       +3     
==========================================
+ Hits         7949     7957       +8     
  Misses       1199     1199              
+ Partials      258      257       -1
Impacted Files Coverage Δ
scrapy/crawler.py 92.39% <100%> (+0.32%) ⬆️
scrapy/utils/trackref.py 86.48% <0%> (+2.7%) ⬆️

@Gallaecio
Copy link
Member Author

@Gallaecio Gallaecio commented Feb 12, 2019

Oh, right, I should cover the checks with automated tests.

@kmike kmike changed the title Check that spidercls arguments in scrapy.crawler classes are not spid… [MRG+1] Check that spidercls arguments in scrapy.crawler classes are not spid… Feb 15, 2019
@kmike
Copy link
Member

@kmike kmike commented Feb 15, 2019

Perfect, thanks @Gallaecio!

@Gallaecio Gallaecio merged commit 70a0f1f into scrapy:master Mar 15, 2019
3 checks passed
@Gallaecio Gallaecio deleted the spidercls-check branch Mar 15, 2019
@kmike kmike added this to the v1.7 milestone Jul 18, 2019
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.

2 participants