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

Initial DynamicDeps support. #201

Merged
merged 10 commits into from
Jul 17, 2024
Merged

Initial DynamicDeps support. #201

merged 10 commits into from
Jul 17, 2024

Conversation

wRAR
Copy link
Member

@wRAR wRAR commented Jun 13, 2024

No description provided.

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.

Not sure if you want to handle the docs now or in a later PR.

@wRAR wRAR requested a review from kmike June 25, 2024 15:36
scrapy_poet/injection.py Outdated Show resolved Hide resolved
scrapy_poet/injection.py Outdated Show resolved Hide resolved
scrapy_poet/injection.py Outdated Show resolved Hide resolved
docs/dynamic-deps.rst Outdated Show resolved Hide resolved
docs/dynamic-deps.rst Outdated Show resolved Hide resolved


def parse_book(self, response, book_page: BookPage, dynamic: DynamicDeps):
other_dep = dynamic[OtherDep]
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to get a list of the created dependencies in the DynamicDeps (i.e. the available keys)? It seems we should document it. Currently only getting by type is documented, if I'm not mistaken.

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed the user already knows which deps are there because it's they who set the meta key, but in other uses cases (which?) they can look at dynamic.keys(), do you want to document this or are you thinking about something else?

Copy link
Member

Choose a reason for hiding this comment

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

There could be a single callback which gets requests from different places, with different "inject" meta; this "inject" meta can be dynamic as well. For example, it can be a list of item classes to extract from a particular page, configured at spider start. Isn't it a point of DynamicDeps that the callback doesn't know what are the dependencies? Otherwise they can be specified in a signature.

As for the documentation, I think documenting DynamicDeps as a dict subclass could be enough.

scrapy_poet/injection.py Outdated Show resolved Hide resolved
scrapy_poet/injection.py Outdated Show resolved Hide resolved
tests/test_injection.py Outdated Show resolved Hide resolved
@kmike
Copy link
Member

kmike commented Jul 12, 2024

Looks great overall 👍

@wRAR wRAR merged commit ba7ac2f into master Jul 17, 2024
16 checks passed
@wRAR wRAR deleted the dynamic-deps branch July 17, 2024 20:53
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