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] Two small fixes for when using the parse command and the '-r' flag (rules). #2225

Merged
merged 4 commits into from
Sep 19, 2016
Merged

[MRG+1] Two small fixes for when using the parse command and the '-r' flag (rules). #2225

merged 4 commits into from
Sep 19, 2016

Conversation

Tethik
Copy link
Contributor

@Tethik Tethik commented Sep 8, 2016

  1. Use default "parse" as callback when the matching rule has no callback.
  2. Log error and return when no rule matches the parsed urln (only if using -r).

The first error was encountered when running something similar to this:
scrapy parse http://www.example.com/blah/ -r
In a project with a CrawlSpider with two rules. The matching rule did not have a callback defined, resulting in the following error when the command tries to fetch the function.

ERROR: Spider error processing <GET http://www.example.com/blah/ (referer: None)
Traceback (most recent call last):
  File "/usr/lib/python3.5/site-packages/twisted/internet/defer.py", line 588, in _runCallbacks
    current.result = callback(current.result, *args, **kw)
  File "/usr/lib/python3.5/site-packages/Scrapy-1.2.0.dev2-py3.5.egg/scrapy/commands/parse.py", line 173, in callback
    cb_method = getattr(spider, cb, None)
TypeError: getattr(): attribute name must be string

1. Use default "parse" as callback when the matching rule has no callback.
2. Log error and return when no rule matches the parsed url.
@codecov-io
Copy link

codecov-io commented Sep 8, 2016

Current coverage is 83.34% (diff: 0.00%)

Merging #2225 into master will decrease coverage by 0.02%

@@             master      #2225   diff @@
==========================================
  Files           161        161          
  Lines          8699       8705     +6   
  Methods           0          0          
  Messages          0          0          
  Branches       1278       1279     +1   
==========================================
+ Hits           7252       7255     +3   
- Misses         1199       1202     +3   
  Partials        248        248          

Powered by Codecov. Last update 0ef570f...8c38dde

@kmike kmike changed the title Two small fixes for when using the parse command and the '-r' flag (rules). [MRG+1] Two small fixes for when using the parse command and the '-r' flag (rules). Sep 8, 2016
@kmike
Copy link
Member

kmike commented Sep 8, 2016

I'm not a scrapy parse user, but the changes look good, thanks!

@redapple
Copy link
Contributor

Thanks @Tethik !
Can I suggest that we add tests for the different cases?
My take on this: https://github.com/Tethik/scrapy/pull/1
(room for factorization)

@Tethik
Copy link
Contributor Author

Tethik commented Sep 12, 2016

@redapple Sounds good, I'll take a look at the your pull with the tests when I next get the time.

@Tethik
Copy link
Contributor Author

Tethik commented Sep 19, 2016

Tests look good to me. I couldn't think of a missing case either. One minor change is that I moved them to a separate file instead (test_commands.py -> test_command_parse.py). I assume codecov doesn't detect them because the tests are generating the spider code on the fly?

Anyhow, I don't have much more input on this. Feel free to merge if you think it looks ok.

Copy link
Contributor

@redapple redapple left a comment

Choose a reason for hiding this comment

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

I'm not sure why Codecov is complaining either.
LGTM.

@redapple
Copy link
Contributor

@kmike , do you (still) agree with the change?

@kmike
Copy link
Member

kmike commented Sep 19, 2016

@redapple yep!
@Tethik hm, I'm not sure coverage problem is caused by generated code (but maybe it is); it looks like our testing suite has problems with subprocesses again.

@kmike kmike merged commit 5523687 into scrapy:master Sep 19, 2016
@kmike
Copy link
Member

kmike commented Sep 19, 2016

Thanks @Tethik!

@kmike kmike added this to the v1.2 milestone Sep 19, 2016
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.

4 participants