-
Notifications
You must be signed in to change notification settings - Fork 10.6k
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
Provide complete API documentation coverage of scrapy.linkextractors #4045
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4045 +/- ##
==========================================
- Coverage 83.85% 83.77% -0.09%
==========================================
Files 165 165
Lines 9615 9634 +19
Branches 1440 1442 +2
==========================================
+ Hits 8063 8071 +8
- Misses 1304 1311 +7
- Partials 248 252 +4
|
Looks good to me, except for the following:
Personally I don't remember seeing a custom Link extractor implementation, but I'm not sure about removing that section. It doesn't hurt, it doesn't need any particular code to be handled and it's technically still supported. |
If we leave it, I would like to at least mention a few use cases, and show a code example. But I cannot think of any use case myself. |
@kmike What do you think, should custom link extractors remain in the documentation or go? |
I like the simplification of the docs, both the wording and content. +1 to deprecate / un-expose FilteringLinkExtractor. As I recall, it was needed when we had several link extractors, but now this code is all way to complex for what it is doing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks @Gallaecio! I left a small comment about a class name used in tests, but otherwise it looks ready to merge 👍
…umentation-coverage
This reverts commit ee9881d.
Thanks @Gallaecio! |
I made the assumption that
FilteringLinkExtractor
was originally created for code sharing between the implementations based onlxml
andsgmllib
.Here I’m marking
FilteringLinkExtractor
as deprecated, assuming that upon removal its code can go intoLxmlLinkExtractor
.I’m also updating the documentation about link extractors to talk about
LxmlLinkExtractor
only, without even mentioning the possibility of alternative implementations.