-
Notifications
You must be signed in to change notification settings - Fork 102
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
Return a Result from Taffy::remove #106
Return a Result from Taffy::remove #106
Conversation
Deciding to unwrap in the tests, so that the tests fail if the remove fails.
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 good and makes sense :)
PR title should be updated to "Return a Result from Taffy::remove" |
@sixfold I broke the merge, sorry! Are you able to fix this? |
Probably. I'll take a look. |
Head branch was pushed to by a user without write access
Should be all fixed now :) |
* Return a result from Sprawl::remove * Update `RELEASES.md` * Fix clippy error, fix compiler warnings Deciding to unwrap in the tests, so that the tests fail if the remove fails. * Run cargo fmt * Move release note to new "0.2.0" section * Add blank lines to satiate the Markdown Lint gods * Remove pointless let _ = call * Fix merge error Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Objective
Fixes: #102
Returning a
Result
here has two nice benefits for users:Sprawl::remove_child
andSprawl::remove_child_at_index
Context
The original issue suggests using an
Option<Id>
for this, but I decided to useResult<usize, Error>
for three reasons:Sprawl::find_node
returns aResult
, and it would be good to propagate that error up.NodeId
is only used internally, so the public API should use the underlyingusize
I also updated the docstring, as returning a
usize
without any additional context could be confusing.Feedback wanted
This is a breaking change. I understand I'm supposed to updateResolvedRELEASES.md
, but the exact details on this aren't super clear. Is what I wrote acceptable?