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

Document the Scrapy component API #5439

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

OrestisKan
Copy link

Remove duplicate definition of from_crawler and from_settings method. Make them refer to the centralized definitions in class-methods.rst where the explanation of the methods is elaborated. Closes #5110

… them refer to the centralized definition of these classmethods locatedin class-methods.rst
@codecov
Copy link

codecov bot commented Mar 7, 2022

Codecov Report

Merging #5439 (42878e0) into master (50c8bec) will decrease coverage by 0.05%.
The diff coverage is n/a.

❗ Current head 42878e0 differs from pull request most recent head 9e0c4f7. Consider uploading reports for the commit 9e0c4f7 to get more accurate results

@@            Coverage Diff             @@
##           master    #5439      +/-   ##
==========================================
- Coverage   88.75%   88.70%   -0.06%     
==========================================
  Files         163      163              
  Lines       10666    10664       -2     
  Branches     1818     1788      -30     
==========================================
- Hits         9467     9459       -8     
- Misses        923      929       +6     
  Partials      276      276              
Impacted Files Coverage Δ
scrapy/core/downloader/handlers/__init__.py 83.63% <0.00%> (-9.10%) ⬇️
scrapy/downloadermiddlewares/cookies.py 95.78% <0.00%> (-2.11%) ⬇️
scrapy/shell.py 67.96% <0.00%> (-0.79%) ⬇️
scrapy/http/response/__init__.py 97.43% <0.00%> (-0.04%) ⬇️
scrapy/http/request/__init__.py 97.77% <0.00%> (-0.03%) ⬇️
scrapy/utils/python.py 87.64% <0.00%> (ø)
scrapy/commands/check.py 71.01% <0.00%> (ø)
scrapy/core/downloader/__init__.py 92.48% <0.00%> (+1.50%) ⬆️

@OrestisKan
Copy link
Author

@Gallaecio Is there anything missing or anything that I could improve?

@Gallaecio
Copy link
Member

Probably not, but I want to take some time to have a proper look at this one. I hope it does not take me too long to find the time.

docs/index.rst Outdated Show resolved Hide resolved
docs/topics/class-methods.rst Outdated Show resolved Hide resolved
docs/topics/class-methods.rst Show resolved Hide resolved
docs/topics/class-methods.rst Show resolved Hide resolved
docs/topics/class-methods.rst Outdated Show resolved Hide resolved
Update as per @Gallaecio 's suggestion, indeed "others" is too generic!

Co-authored-by: Adrián Chaves <adrian@chaves.io>
@OrestisKan OrestisKan marked this pull request as draft March 17, 2022 15:06
atatabitovska and others added 5 commits March 18, 2022 14:41
Update class-methods.rst

Resolve inconsistent vocabulary. Add explanation on from settings
re-worded, i'll try to run it now and see how it acts
@OrestisKan OrestisKan marked this pull request as ready for review March 20, 2022 12:00
@OrestisKan
Copy link
Author

@Gallaecio the changes that you requested are done, please let me know if there is anything I can improve

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

Sorry for the late feedback. I will try to be quicker for any further feedback.

Comment on lines +1 to +3
===========================
Class Factory Methods
===========================
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
===========================
Class Factory Methods
===========================
=====================
Class Factory Methods
=====================

Comment on lines +191 to +196
:meth:`from_settings`:

This class method is used by Scrapy to create an instance of the class.
It's called with the current project settings, and it loads the spiders
found recursively in the modules of the :setting:`SPIDER_MODULES`
setting.
Copy link
Member

Choose a reason for hiding this comment

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

I am thinking factory method signatures should still be kept in the API reference. It is specially important because some components may get extra arguments, I believe, and in those cases we should document them in one of the methods (e.g. from_crawler) and in the other method refer to the former for parameter details.

What we can do now is simplify the description, both of from_crawler and from_settings, to something like:

Factory method. See :ref:`class-methods`.

Assuming we also add .. _class-methods: at the beginning of the new topic.

Also, I imagine (please check) that this class also supports from_crawler. In that case, we should include an entry for that class method as well, with its own signature, but the same description that basically points to the new topic.

Comment on lines +11 to +21
The ``from_crawler`` class method is implemented in the following objects:
* ItemPipeline
* DownloaderMiddleware
* SpiderMiddleware
* Scheduler
* BaseScheduler
* Spider

The ``from_settings`` class method is implemented in the following objects:
* MailSender
* SpiderLoader
Copy link
Member

Choose a reason for hiding this comment

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

Some of these are classes, some are interface definitions (they define an API, but do not really exist as classes in Python code), but none of them are objects.

This also seems to suggest that the upper list supports only from_crawler and the lower list supports only from_settings. Is that so?

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.

Document the Scrapy component API
3 participants