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

make_request_from_data implementation in RedisMixin #267

Open
rjbks opened this issue Jan 24, 2023 · 2 comments
Open

make_request_from_data implementation in RedisMixin #267

rjbks opened this issue Jan 24, 2023 · 2 comments
Labels

Comments

@rjbks
Copy link

rjbks commented Jan 24, 2023

I was wonder why the make_request_from_data method doesn't just use the more full featured request_from_dict function from scrapy utils that the queue class in the library already uses? It seems that both do the same thing but even the documentation is a bit misleading when it says:

this data can be accessed in scrapy spider through response. like: request.url, request.meta, request.cookies

as nothing outside of url, method, meta get set as keyword args anyway.

@rmax
Copy link
Owner

rmax commented Jan 31, 2023

At the time, request_from_dict required several keys as you can see here. And the need was to load start URLs from redis, which initially were just URL strings, and then changed to dict-like with url, method and meta which were the fields some users needed.

I'm not sure if current request_from_dict can replace current approach seamlessly, feel free to contribute!

@rmax rmax added the question label Jan 31, 2023
@LuckyPigeon
Copy link
Collaborator

IMO, we don't need request_from_dict at the moment. The very first concept of make_request_from_data is making our request more flexible, on the other hand, request_from_dict is letting users define and pass their own request class.

For the above reason, I don't think that we should add request_from_dict now. But discussions are welcome, and feel free to contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants