Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

Abdelgha-4
Copy link

@Abdelgha-4 Abdelgha-4 commented Jun 8, 2025

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:

print(isinstance(True, int))
>>> True

Please note that the same issue may cause some other parts to function unexpectedly, for example:

    def __get_type_checked_text(self, text):
        ...
        elif isinstance(text, (int, float)):
            return str(text)  # Convert num to string
        elif isinstance(text, bool):
            raise Exception("text must be a string! Boolean found!")

isinstance(text, bool) will never be reached and passing text=True or text=False will just return "True" or "False"

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
```
@Abdelgha-4 Abdelgha-4 changed the title Fix load_cookies behavior when expiry=True Fix load_cookies, add_cookie & add_cookies behavior when expiry=True Jun 8, 2025
@mdmintz
Copy link
Member

mdmintz commented Jun 9, 2025

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.

@mdmintz mdmintz closed this Jun 9, 2025
@Abdelgha-4
Copy link
Author

Abdelgha-4 commented Jun 9, 2025

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!

@mdmintz
Copy link
Member

mdmintz commented Jun 9, 2025

__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.

@Abdelgha-4
Copy link
Author

Abdelgha-4 commented Jun 9, 2025

@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.

@mdmintz
Copy link
Member

mdmintz commented Jun 9, 2025

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.

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