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] settings: fixing name of the pipeline template #2466

Merged
merged 1 commit into from Feb 2, 2017

Conversation

@eLRuLL
Copy link
Member

@eLRuLL eLRuLL commented Dec 24, 2016

scrapy startproject myproject creates a pipelines.py with an example Pipeline with already the name of the project, it only feels natural that this should also be the name on settings.py pipelines section.

@codecov-io
Copy link

@codecov-io codecov-io commented Dec 24, 2016

Current coverage is 83.44% (diff: 100%)

Merging #2466 into master will not change coverage

@@             master      #2466   diff @@
==========================================
  Files           161        161          
  Lines          8779       8779          
  Methods           0          0          
  Messages          0          0          
  Branches       1288       1288          
==========================================
  Hits           7326       7326          
  Misses         1205       1205          
  Partials        248        248          

Powered by Codecov. Last update cc06b6b...e7c7e05

Copy link
Contributor

@redapple redapple left a comment

Makes sense @eLRuLL !
+1 for me

@redapple redapple changed the title settings: fixing name of the pipeline template [MRG+1] settings: fixing name of the pipeline template Jan 3, 2017
@kmike
Copy link
Member

@kmike kmike commented Jan 8, 2017

I don't have a strong opinion on that - on one hand, this adds some convenience when there is a single pipeline, but on the other hand creating single-purpose components with descriptive names is usually better.

@redapple
Copy link
Contributor

@redapple redapple commented Jan 10, 2017

I think the project template needs to be consistent in naming, and the generated settings need to be coherent with the generated classes.
Currently, in settings.py, there's:

  • $project_name.middlewares.${ProjectName}SpiderMiddleware, which matches the generated class ${ProjectName}SpiderMiddleware(object) in middlewares.py
  • $project_name.middlewares.MyCustomDownloaderMiddleware, which is not in the template,
  • $project_name.pipelines.SomePipeline which does not match the generated class ${ProjectName}Pipeline(object) in pipelines.py

So it's far from using descriptive component names (I agree with @kmike that well-named single-purpose components is good advice, but it may be something for the docs or via comments in the generated files?), nor is it coherent.
And there could also be a class ${ProjectName}DownloaderMiddleware(object) in middlewares.py even.

@eLRuLL
Copy link
Member Author

@eLRuLL eLRuLL commented Jan 12, 2017

I agree with you @kmike but I think that isn't the case here. I am just changing the SomePipeline name that was already in the project without any real purpose. I would say it actually could confuse a beginner.

@kmike kmike merged commit 9c75403 into scrapy:master Feb 2, 2017
3 checks passed
3 checks passed
codecov/patch Coverage not affected when comparing cc06b6b...e7c7e05
Details
codecov/project 83.44% (+0.00%) compared to cc06b6b
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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants