Conversation
src/lib.rs
Outdated
| pub struct NodeNotFoundError(pub node::Node); | ||
|
|
||
| #[cfg(feature = "std")] | ||
| impl Display for NodeNotFoundError { |
There was a problem hiding this comment.
I think we should just pull in thiserror and feature-gate it behind std. It's a much nicer way to define these error strings.
src/lib.rs
Outdated
| child_count: usize, | ||
| }, | ||
| ///The [`Node`](node::Node) was not found in the [`Taffy`] instance | ||
| NodeNotFoundError(NodeNotFoundError), |
There was a problem hiding this comment.
I don't love nesting these errors like this, but until we have "enum variants are types" this is a better design to keep the function signatures of most of the operations cleaner.
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Thanks! First round of feedback is up :) Some suggestions on naming mostly.
I'd like feedback on whether or not we should use thiserror.
The API is much nicer, but it pulls in some relatively heavy proc-macro crates which might hurt compile times.
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Almost ready now.
|
Excellent work; thanks for being so responsive to reviews. |
|
Thank you for being quick to provide feedback. I appreciate it |
* Added checks for child_index out of bounds * slightly better descriptions * updated RELEASES.md * Changed TaffyError back to Error * split error type into two * fixed error in git merge * updated RELEASES.md file * cargo fmt * moved errors into error module, and gave them better names * Split InvalidChild::InvalidNode into two * updated RELEASES.md * cargo fmt * more changes to RELEASES.md Co-authored-by: Alice Cecile <[email protected]>
2 participants
Objective
Replaces
taffy::Errorwithtaffy::NodeNotFoundErrorto make code much more legible. Also addstaffy::ChildOperationErrorso thatremove_child_at_index,replace_child_at_index, andchild_at_indexcan return an error instead of panicing.Fixes #100 #104
Feedback wanted
I'm not happy with the name of
ChildOperationError. It feels a little vague to me. Is it fine, or is there possibly a better name for it?Also, I'm not sure I should have flattened NodeNotFoundError into a tuple struct.