-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix load_cookies
, add_cookie
& add_cookies
behavior when expiry=True
#3803
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
Conversation
The statement `isinstance(expiry, (int, float)) and expiry > 0` evaluates to `True` when `expiry=True` due to booleans being a subclass of integers in python: ```python print(isinstance(True, int)) >>> True ```
load_cookies
behavior when expiry=True
(削除ここまで)load_cookies
, add_cookie
& add_cookies
behavior when expiry=True
(追記ここまで)
Hello. Thanks for reporting the issue. I wanted to implement the fix differently. Your update would've failed the flake8
verification check. The fix for handling expiry
has been implemented in 8471ef1.
Hello @mdmintz, you're welcome!
Your fix did not solve the issue I mentioned with __get_type_checked_text
, I didn't scan the full codebase for similar bugs too, but you'd know better all the places where you maybe check for bool vs integer types.
Btw I wouldn't mind applying any needed change if you could've pointed out what you'd like to be done differently in the PR, you'd have less things to implement on your own and more people contributing to the project and learning from the experience, the project and from yourself!
__get_type_checked_text()
is just for string inputs.
It will convert True
to "True"
and False
to "False"
, as expected.
As for contributions, we've mostly moved to the policy of https://github.com/readme/featured/how-open-is-open-source, with some special exceptions. ("Open-Source, not Open-Contribution"). We still welcome feedback, issue tickets, discussions, etc, but are very picky about outside contributions, as that takes a lot of extra time, which I don't have much of due to a full-time job, etc. If you think you see an issue, it's best to report it, rather than try to create a PR.
@mdmintz For __get_type_checked_text()
, then this block could be removed from the function:
elif isinstance(text, bool): raise Exception("text must be a string! Boolean found!")
For contributions, good to know, in all cases it's great work you'll pulling up here.
Valid point on the True
being both a bool
and an int
, but I may just leave it in there so that people who don't realize it don't complain that it's not being checked for, event though True and False will get converted to strings in actuality. It's a rare case that would either have to throw an exception, or just convert to text as it does to avoid the exception.
Uh oh!
There was an error while loading. Please reload this page.
The statement
isinstance(expiry, (int, float)) and expiry > 0
evaluates toTrue
whenexpiry=True
due to booleans being a subclass of integers in python:Please note that the same issue may cause some other parts to function unexpectedly, for example:
isinstance(text, bool)
will never be reached and passingtext=True
ortext=False
will just return"True"
or"False"