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

BooleanWidget optimization #1612

Conversation

walison17
Copy link

Small optimization on lookups using sets instead lists

@andrewgy8
Copy link
Member

andrewgy8 commented Sep 1, 2023

Hey! Thanks for the contrib @walison17 !

Can you explain more about why you think this optimization is necessary? Did you run into an issue with the speed of this function?

This change seems like a non-backwards compatible change, so there is a significant cost here that we must weigh.

@walison17
Copy link
Author

hey @andrewgy8!

I wrote the FALSE_RENDER_VALUE wrong before sending this PR, was not my intention to add a breaking change on returned value, sorry about that 😅

@matthewhegarty
Copy link
Contributor

matthewhegarty commented Sep 6, 2023

Thanks for submitting the PR. I agree with @andrewgy8 that there's not enough of a benefit to merge this PR. Unless there's something else I am missing, I think the performance improvement would be negligible. If we put this change in, there is a risk that we will break existing implementations out there, i.e. if users have their own code which relies on using the 0th element of the lists.

I will close this but please note that we appreciate you creating a PR, and we encourage you to looks through existing issues and feel free to pick an issue if you want to contribute to the library. It's always worth adding comments before undertaking significant work, so that we can let you know if we think it's the right approach.

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.

None yet

3 participants