-
-
Notifications
You must be signed in to change notification settings - Fork 587
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
change: tweak API of Errors
and implement IntoIter
#522
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me, The tricky part as always is making the internal HashMap private. We have remove, insert, and
.into_iter() | ||
.filter_map(|(_k, v)| v.downcast_ref::<TodoAppError>().cloned()) | ||
.filter_map(|(_, v)| v.downcast_ref::<TodoAppError>().cloned()) | ||
.collect(); | ||
println!("Errors: {errors:#?}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably did this, but we can remove this println!
#[derive(Debug, Default, Clone, PartialEq, Eq, Hash)] | ||
pub struct ErrorKey(String); | ||
|
||
impl<T> From<T> for ErrorKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good, but I should ask whether we make any assumptions elsewhere that this is a valid HydrationKey? I set it outside leptos with a default for 404, but is there a cleanup function somewhere that might need this to be valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same rough idea, making this private is probably the right way to go, but there are a lot of possible things people can do to a HashMap. As long as we're okay with limiting those, this is fine.
I've been thinking about how to limit the API surface of
Errors
so we aren't committed to any particular internal implementation. I'm hoping to achieve the following with this PR:HashMap
forever without being able to switch to another data structureIntoIter
so you can just do thiserrors.get().0
and friends@benwis I'd like you to review this because a) it's your creation originally and b) without a doubt you're using this in more places than anyone else. Remember it's a lot easier for us to add additional methods in the future (not a breaking change to add a method) than to remove them or change them (breaking changes).