-
Couldn't load subscription status.
- Fork 28
url-matcher integration with scrapy-poet #56
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
Conversation
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.
Hi @ivanprado , added some comments after a first pass. Looking good so far!
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.
LGTM 👍
tox.ini
Outdated
| scrapy >= 2.1.0 | ||
| pytest-twisted | ||
| web-poet | ||
| web-poet @ git+https://[email protected]/scrapinghub/web-poet@handle_urls#egg=web-poet |
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.
Reminder to update this when scrapinghub/web-poet#16 is released.
Codecov Report
@@ Coverage Diff @@
## master #56 +/- ##
==========================================
+ Coverage 95.96% 97.46% +1.50%
==========================================
Files 9 9
Lines 372 395 +23
==========================================
+ Hits 357 385 +28
+ Misses 15 10 -5
|
54ea603 to
63029dc
Compare
7e00bf6 to
17689b5
Compare
| BookPage: BPBookPage | ||
| }, | ||
| } | ||
| "SCRAPY_POET_OVERRIDES": [ |
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.
I wonder if we should provide an example with handle_urls decorator
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.
A good point! Added such an example in 0bc51b8.
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.
Thanks @BurnzZ, @ivanprado and @sortafreel!
I left a couple of minor comments; the PR looks good to me, +1 to merge.
4374ad7 to
0bc51b8
Compare
WARNING: this branch was created on top of #55. It should be merged only after the former one has been merged
New registry that uses the
url-matcherpatterns to configure the overrides. It is a backwards-incompatible change. See the updated documentation to get more context.A new release of web-poet is required because this depends on
scrapinghub/web-poet#16scrapinghub/web-poet#27TODO:
tox.iniandsetup-pyonce a new version is releasedOverrideRule,PageObjectRegistry, etc. using:ref:directive. But it requires introduce PageObjectRegistry with @hande_urls annotations web-poet#27 to be merged first.