Enforce Datetime Type for Expires on Set-Cookie #1484
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1484 +/- ##
==========================================
+ Coverage 91.44% 91.45% +<.01%
==========================================
Files 17 17
Lines 1730 1731 +1
Branches 328 330 +2
==========================================
+ Hits 1582 1583 +1
Misses 123 123
Partials 25 25
Continue to review full report at Codecov.
|
|
The CI appears to be failing for a reason outside this PR scope. It's a lint error on sanic/response.py. Let me know if this is related to my PR or if you guys want me to fix it, I would be glad to help! |
|
Thanks for your effort, @LTMenezes ! I guess this is something that was already been discussed, I just don't remember if there was any conclusions ... @harshanarayana , any thoughts? |
|
@vltr This is something we discussed in the forum. The conclusion was to let datetime be used as a valid value and raise an exception otherwise to inform that the type is invalid. (Enables easy date to string conversion in RFC Standard) |
|
@LTMenezes I looked at the travis jobs. would reformat /home/travis/build/huge-success/sanic/sanic/cookies.pyLooks like |
sanic/cookies.py
Outdated
| @@ -1,5 +1,6 @@ | |||
| import re | |||
| import string | |||
| import datetime | |||
There was a problem hiding this comment.
Can we just import what we need for the check here? from datetime import datetime
sanic/cookies.py
Outdated
| @@ -108,6 +109,11 @@ def __setitem__(self, key, value): | |||
| if key.lower() == "max-age": | |||
| if not str(value).isdigit(): | |||
| value = DEFAULT_MAX_AGE | |||
| elif key.lower() == "expires": | |||
| if type(value) is not datetime.datetime: | |||
There was a problem hiding this comment.
type(x) can probably be replaced with isinstance(x, datetime) instead.
sanic/cookies.py
Outdated
| @@ -108,6 +109,11 @@ def __setitem__(self, key, value): | |||
| if key.lower() == "max-age": | |||
| if not str(value).isdigit(): | |||
| value = DEFAULT_MAX_AGE | |||
| elif key.lower() == "expires": | |||
| if type(value) is not datetime.datetime: | |||
| raise KeyError( | |||
There was a problem hiding this comment.
This is more of a TypeError rather than a KeyError
|
Thanks for the review @harshanarayana! I applied the suggestions you made and fixed the build. I also committed other files that the 'make beautify' command formatted, hope that this is okay. |
There was a problem hiding this comment.
LGTM
@LTMenezes Looks like you reformatted the tests too. No worries I guess. Hope @huge-success/sanic-core-devs is okay with this.
|
Looks ok to me too. |
Closes: #1453