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

Remove the cache from the provider. #172

Closed
wants to merge 1 commit into from
Closed

Conversation

wRAR
Copy link
Contributor

@wRAR wRAR commented Feb 5, 2024

This removes the cache added in #94. It's possible that there are use cases that I don't see (and which aren't covered by tests) where it's used, but it looks like we shouldn't call the provider multiple times for the same request?

@wRAR wRAR requested a review from Gallaecio February 5, 2024 11:37
@Gallaecio Gallaecio requested a review from BurnzZ February 5, 2024 12:04
@Gallaecio
Copy link
Contributor

I imagine this was necessary before your item building refactoring, where resolution happened at different stages and the same provider could be called multiple times, and it is not needed anymore. But I suspect @BurnzZ might have a better view here (or/and I might be mixing different caches in my mind).

@BurnzZ
Copy link
Member

BurnzZ commented Feb 5, 2024

This coincides with #161 where we still kinda need the cache, albeit replaced with the new one in scrapinghub/scrapy-poet#184.

@BurnzZ
Copy link
Member

BurnzZ commented Feb 5, 2024

Ahh I think the lines like self.update_cache(request, {BrowserHtml: html}) aren't needed anymore. Perhaps it's best to have this after #161 is merged?

@wRAR
Copy link
Contributor Author

wRAR commented Feb 5, 2024

Do we only need the scrapy-poet one then?

@wRAR wRAR marked this pull request as draft February 5, 2024 12:10
@BurnzZ BurnzZ mentioned this pull request Feb 7, 2024
4 tasks
@kmike
Copy link
Member

kmike commented Feb 16, 2024

Is this still valid?

@wRAR wRAR marked this pull request as ready for review February 16, 2024 20:40
@wRAR
Copy link
Contributor Author

wRAR commented Mar 4, 2024

Done in #161

@wRAR wRAR closed this Mar 4, 2024
@wRAR wRAR deleted the remove-provider-cache branch March 4, 2024 08:36
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.

4 participants