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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Annotate samesite parameter on set_cookie #1590

Merged
merged 6 commits into from Apr 26, 2022
Merged

Annotate samesite parameter on set_cookie #1590

merged 6 commits into from Apr 26, 2022

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Apr 15, 2022

No description provided.

@Kludex Kludex mentioned this pull request Apr 15, 2022
2 tasks
Copy link
Member

@euri10 euri10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably weird question but shouldn't it be Lax Strict and None or we lowercase everything and it doesnt make a difference ?

@Kludex
Copy link
Sponsor Member Author

Kludex commented Apr 18, 2022

Good question. We lower case anyway on set_cookie, but not on the SessionMiddleware. As per https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-cookie-same-site-00#section-4.1 , both are accepted.

Maybe the right type would be Literal["lax", "Lax", "none", "None", "strict", "Strict"]. At this point, is it better to let str? Maybe just documenting the values somewhere in the docs?

@Kludex Kludex added this to the Version 0.20.0 milestone Apr 21, 2022
@Kludex Kludex mentioned this pull request Apr 22, 2022
2 tasks
@Kludex Kludex added the typing Type annotations or mypy issues label Apr 24, 2022
@Kludex Kludex requested a review from a team April 24, 2022 06:01
@adriangb
Copy link
Member

I think this is good. It helps users but also developers so we can know what values are accepted just by looking at it.

I think we should just type it all as lowercase, even if at runtime we accept both.

Side note: would it make sense to have a _typing.py module for this sort of thing?

@Kludex
Copy link
Sponsor Member Author

Kludex commented Apr 24, 2022

Side note: would it make sense to have a _typing.py module for this sort of thing?

Not sure... Just for the type added here is enough? 馃 Maybe... Is it the only type that could be added there right now? 馃

In any case, I guess is not a blocker for this PR. I'll wait to see if someone has any other comment about this PR, and merge it before 0.20.0.

@adriangb
Copy link
Member

Just for this it doesn't make sense, but using that argument it would never make sense. I think there are other shared things like this scattered around the codebase? And maybe some of the stuff in typing.py and datastructructures.py could go in there? Just a thought...

@Kludex Kludex merged commit 9a1e885 into master Apr 26, 2022
@Kludex Kludex deleted the annotate-samesite branch April 26, 2022 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typing Type annotations or mypy issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants