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] command-list-fix #2695

Merged
merged 2 commits into from Apr 5, 2017
Merged

[MRG+1] command-list-fix #2695

merged 2 commits into from Apr 5, 2017

Conversation

@LMKight
Copy link
Contributor

@LMKight LMKight commented Apr 2, 2017

Remove "commands" from the command list (the one that gets printed when you type e.g. "scrapy" in the command line), since that is the baseclass of all commands and cannot be executed.

@codecov-io
Copy link

@codecov-io codecov-io commented Apr 2, 2017

Codecov Report

Merging #2695 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2695   +/-   ##
=======================================
  Coverage   84.66%   84.66%           
=======================================
  Files         162      162           
  Lines        9122     9122           
  Branches     1353     1353           
=======================================
  Hits         7723     7723           
  Misses       1141     1141           
  Partials      258      258
Impacted Files Coverage Δ
scrapy/cmdline.py 66.4% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8753b45...05ce129. Read the comment docs.

@kmike
Copy link
Member

@kmike kmike commented Apr 3, 2017

A good catch!

I'm not sure I like the fix though - it is non-obvious what this code does. The goal is to exclude base ScrapyCommand class from the list, right? It seems a more clear fix is to exclude it in _iter_command_classes: issubclass(ScrapyCommand, ScrapyCommand) is True, so an extra check is needed.

@kmike kmike changed the title command-list-fix {MRG+1] command-list-fix Apr 5, 2017
@kmike kmike changed the title {MRG+1] command-list-fix [MRG+1] command-list-fix Apr 5, 2017
@kmike
Copy link
Member

@kmike kmike commented Apr 5, 2017

Looks good to me, thanks @LMKight!

@redapple redapple merged commit 2528191 into scrapy:master Apr 5, 2017
3 checks passed
3 checks passed
codecov/patch Coverage not affected when comparing 8753b45...05ce129
Details
codecov/project 84.66% remains the same compared to 8753b45
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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