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

move some registry functionalities from scrapy-poet #112

Merged
merged 16 commits into from
Jan 16, 2023
Merged

Conversation

BurnzZ
Copy link
Contributor

@BurnzZ BurnzZ commented Jan 6, 2023

Taken from scrapinghub/scrapy-poet#103.

TODO:

  • Fix flaky tests
  • Optimize
  • Docs
  • Changelog

@codecov
Copy link

codecov bot commented Jan 6, 2023

Codecov Report

Merging #112 (e86bf37) into master (a44c426) will decrease coverage by 0.29%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
- Coverage   99.85%   99.56%   -0.30%     
==========================================
  Files          22       25       +3     
  Lines         697      921     +224     
==========================================
+ Hits          696      917     +221     
- Misses          1        4       +3     
Impacted Files Coverage Δ
web_poet/rules.py 100.00% <100.00%> (ø)
web_poet/utils.py 100.00% <0.00%> (ø)
web_poet/serialization/api.py 100.00% <0.00%> (ø)
web_poet/testing/fixture.py 99.04% <0.00%> (ø)
web_poet/testing/pytest.py 96.55% <0.00%> (ø)
web_poet/testing/__init__.py 100.00% <0.00%> (ø)

web_poet/rules.py Outdated Show resolved Hide resolved
web_poet/rules.py Outdated Show resolved Hide resolved
@BurnzZ BurnzZ force-pushed the registry-features branch from b699bd8 to a59bd21 Compare January 9, 2023 09:03
@BurnzZ BurnzZ changed the title WIP: move some registry functionalities from scrapy-poet move some registry functionalities from scrapy-poet Jan 9, 2023
@BurnzZ BurnzZ marked this pull request as ready for review January 9, 2023 10:20
@BurnzZ BurnzZ requested review from Gallaecio and wRAR January 9, 2023 10:20
web_poet/rules.py Outdated Show resolved Hide resolved
web_poet/rules.py Outdated Show resolved Hide resolved
web_poet/rules.py Outdated Show resolved Hide resolved
web_poet/rules.py Outdated Show resolved Hide resolved
web_poet/rules.py Outdated Show resolved Hide resolved
web_poet/rules.py Outdated Show resolved Hide resolved
web_poet/rules.py Outdated Show resolved Hide resolved

from web_poet import default_registry

item_cls = default_registry.page_object_for_item(
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it be page_cls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean renaming it something like page_object_cls_for_item? or maybe page_cls_for_item?

Copy link
Member

Choose a reason for hiding this comment

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

The varible is named item_cls, but it contains page class, not item class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally missed that. Thanks! Updated. 1ab1a43

Copy link
Member

Choose a reason for hiding this comment

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

You actually raised a good point - should we rename this method to page_cls_for_item, as it doesn't return an instantiated object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think that's much better. e86bf37

Copy link
Member

@kmike kmike left a comment

Choose a reason for hiding this comment

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

👍

@kmike kmike merged commit d5f63d8 into master Jan 16, 2023
@kmike
Copy link
Member

kmike commented Jan 16, 2023

Thanks @BurnzZ!

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.

3 participants