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

WIP: CookiesMiddleware: add "reset_cookies" meta to clear the jar #2986

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

immerrr
Copy link
Contributor

@immerrr immerrr commented Oct 30, 2017

This PR adds a reset_cookies meta to clean the active cookiejar.

When I try working with sessions I often find myself in a situation when I'd like to restart the session "from scratch," including cookies. It's doable by just setting cookiejar meta to an arbitrary value, but then we'd be accumulating the cookiejars over time.

I'm not entirely sure about the meta name, as I'd probably like it namespaced, e.g. cookies_reset, but there's a precedent already with dont_merge_cookies, so I chose to follow that pattern.

@kmike am I missing something here?

@immerrr immerrr changed the title CookiesMiddleware: add "reset_cookies" meta to clear the jar WIP: CookiesMiddleware: add "reset_cookies" meta to clear the jar Oct 30, 2017
@codecov
Copy link

codecov bot commented Oct 30, 2017

Codecov Report

Merging #2986 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2986      +/-   ##
==========================================
- Coverage   84.59%   84.57%   -0.02%     
==========================================
  Files         164      164              
  Lines        9249     9251       +2     
  Branches     1376     1377       +1     
==========================================
  Hits         7824     7824              
- Misses       1167     1168       +1     
- Partials      258      259       +1
Impacted Files Coverage Δ
scrapy/downloadermiddlewares/cookies.py 94.02% <0%> (-2.9%) ⬇️

@kmike
Copy link
Member

kmike commented Oct 31, 2017

I'd love to see cookiejar exposed to user directly instead of adding these "commands" in request.meta (thought on #1878 are welcome!). What's the use case? Note that with reset_cookies you're restarting from scratch at arbitrary point of time, when Request is started to download, not when you send a Request. Is it what you really want, or do you really want to clear cookiejar when sending a request?

If we're going down this request.meta route, the implementation looks fine. As for the name, what about cookiejar_reset, to use some namespace? We have many names prefixed with dont_, but other prefixes ar not common.

@immerrr
Copy link
Contributor Author

immerrr commented Oct 31, 2017

What's the use case?

I think I've mentioned it in the OP, I'm doing sessions on top of Crawlera and want to clean up "banned" cookiejars instead of generating new ones and accumulating the old ones over time.

Request is started to download, not when you send a Request. Is it what you really want, or do you really want to clear cookiejar when sending a request?

I don't care much about it, as long as the outgoing request is clean it's fine.

@immerrr
Copy link
Contributor Author

immerrr commented Jan 21, 2018

@kmike anything else to fix here? (aside from adding tests)

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.

None yet

2 participants