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

Add transpose to the OptionStoreExt #3534

Merged
merged 4 commits into from
Feb 14, 2025

Conversation

mahdi739
Copy link
Contributor

This PR adds a method called "transpose" to the OptionStoreExt which converts a subfield with an option into an option of a subfield.
I found it useful in pattern matching while we want to keep the reactivity of the subfield.

@gbj
Copy link
Collaborator

gbj commented Feb 1, 2025

Nice! Semver is going to complain here because it adds a new method to the trait, which could in theory be implemented somewhere else. It won't complain if there's a default implementation, though... And I'm pretty sure this can be default-implemented as self.map(|value| value), right?

(In a more general sense, .map() is really just self.transpose().map(map_fn), I guess.)

I guess my one qualm is that transpose() does already exist as a name for something to do with Option<_>, which transposes an Option<Result<T, E>> to Result<Option<T>, E>. On the other hand, this does convey the same sense of "we rearranged the monads!" so maybe it's a fine name after all.

@mahdi739
Copy link
Contributor Author

mahdi739 commented Feb 1, 2025

Thanks. Actually I wasn't sure about the name so I sent it to get some input. And maybe with the presence of map it's kind of redundant. You can close it or change it or tell me if I should make any change.

@mahdi739
Copy link
Contributor Author

I made several changes based on your feedback:

  • Implemented the new method as a default implementation in the trait to avoid breaking changes.
  • Renamed transpose to invert to distinguish it from Rust's similar method.
  • Replaced read with try_read in map and map_untracked to handle cases where the signal was disposed, now it returns None in such scenarios.

@gbj
Copy link
Collaborator

gbj commented Feb 14, 2025

Awesome. Looks great -- thank you!

@gbj gbj merged commit 68f4d46 into leptos-rs:main Feb 14, 2025
74 checks passed
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