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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove mutable defaults and allow more generic types #357

Merged

Conversation

sebastian-correa
Copy link
Collaborator

Summary

Solve #353 and #354 by removing mutable defaults and allowing more generic types than just lists/tuples/sets.

Details

Mutable defaults

Mutable defaults are a bad idea since they're evaluated at definition time and mutating a default value in 1 instance will mutate it in all of them.

This PR solves bad mutable defaults by allowing the user to pass (and defaulting to) None, which retains the old behavior.

More generic types

The code already worked for any Iterables, as we only iterated on the values. These changes reflect that reality by not forcing the user to pass lists, but allowing them to pass any iterables and storing them as lists internally (to not change the API).

str_types deprecation

str_types is a legacy thing from when the library supported Python 2, where the str_types were str and unicode. This PR deprecates the variable in favour of checking against str. This is a minor internal tweak.

Questions

I replaced the AssertionErrors with more aptly (IMO) suited TypeErrors and ValueErrors. Do you think this constitute a breaking change? Given the errors were raised only upon instantiation for validation and that they were undocumented behaviour, I'd say these are not breaking.

Notes

I'm totally open to any critique/requests, and even to not merging the PR if you think it's not a good idea. Just let me know!

@melroy89
Copy link
Collaborator

Thanks once again for the PR! I will look tomorrow into it.

@melroy89 melroy89 self-requested a review September 30, 2024 11:06
Copy link
Collaborator

@melroy89 melroy89 left a comment

Choose a reason for hiding this comment

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

LGTM. I will test it locally a bit version. EDIT: Looks good! 🥳

Ps. Sorry I couldn't test it earlier. There was some kind of Chinese & Singapore DDoS attack happening. Hopefully not using fake-useragent python package.. 🤕

@melroy89 melroy89 enabled auto-merge (squash) September 30, 2024 15:10
@melroy89
Copy link
Collaborator

Also this is no longer a problem indeed:

>>> ua3.browsers
['chrome', 'firefox', 'safari', 'edge']
>>> ua3.browsers.append("opera")
>>> ua3.browsers
['chrome', 'firefox', 'safari', 'edge', 'opera']
>>> ua2 = FakeUserAgent()
>>> ua2.browsers
['chrome', 'firefox', 'safari', 'edge']

@melroy89 melroy89 merged commit 6ddbb4d into fake-useragent:main Sep 30, 2024
10 checks passed
@sebastian-correa
Copy link
Collaborator Author

Ps. Sorry I couldn't test it earlier. There was some kind of Chinese & Singapore DDoS attack happening. Hopefully not using fake-useragent python package.. 🤕

Haha, let's hope not. Thanks for the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants