Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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
Abdelgha-4 wants to merge 2 commits into seleniumbase:master from Abdelgha-4:patch-2

Conversation

Copy link

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

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"

Abdelgha-4 added 2 commits June 8, 2025 19:07
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
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.

Copy link
Author

Abdelgha-4 commented Jun 9, 2025
edited
Loading

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!

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.

Copy link
Author

Abdelgha-4 commented Jun 9, 2025
edited
Loading

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

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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /