rmax / scrapy-redis

Redis-based components for Scrapy.
http://scrapy-redis.readthedocs.io
MIT License
5.49k stars 1.59k forks source link

make_request_from_data implementation in RedisMixin #267

Open rjbks opened 1 year ago

rjbks commented 1 year ago

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 commented 1 year ago

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!

LuckyPigeon commented 1 year ago

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.