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

Response.delete_cookie accepts cookie options #1228

Merged
merged 3 commits into from
Oct 5, 2021
Merged

Conversation

BookGin
Copy link
Contributor

@BookGin BookGin commented Jul 2, 2021

Though functools.partial could be used, it needs to be instantiated somewhere. Otherwise it will introduce the overhead of function object every time.

Let's just follow werkzeug's implementation.

@BookGin BookGin changed the title Fix #1207 Response.delete_cookie accpets cookie options Jul 2, 2021
@Kludex
Copy link
Member

Kludex commented Sep 25, 2021

@BookGin What do you mean by: "Though functools.partial could be used, it needs to be instantiated somewhere."?

Thanks for the PR! 🎉

@BookGin
Copy link
Contributor Author

BookGin commented Sep 28, 2021

Like this pseudo-code, so we need to instantiate the partial function somewhere.....

from functools import partial
def delete_cookie(
       self,
       key: str,
       path: str = "/",
       domain: str = None,
       secure: bool = False,
       httponly: bool = False,
       samesite: str = "lax",
   ) -> None:
   if not hasattr(delete_cookie, '_func'):
       delete_cookie._func = partial(self.set_cookie, max_age=0, expires=0)
   return delete_cookie._func(path, domain, secure, httponly, samesite)

Never mind. I don't think it's more readable. Let's just keep things simple.

@Kludex Kludex changed the title Response.delete_cookie accpets cookie options Response.delete_cookie accepts cookie options Sep 28, 2021
@Kludex
Copy link
Member

Kludex commented Oct 5, 2021

@BookGin Thanks for the PR! 🎉

@Kludex Kludex merged commit ada99be into encode:master Oct 5, 2021
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.

2 participants