-
Notifications
You must be signed in to change notification settings - Fork 896
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 Result from Shape methods #6398
Conversation
IMO, using the last successfully modified width is fine. Actually, that might be the more accurate width in some cases. |
by the way @ding-young thank you for all your excellent work this year! I know you worked closely with @ytmimi and we didn't get a chance to interact much, but i really appreciate all the work you did and I'm glad that you have expressed an interest in remaining engaged with it |
@camsteffen - re: this item, i've not looked at the diff yet but if there's a change to the formatting output (even if it's an improvement) then we'll need to edition gate it behind style edition 2027 e.g. Line 147 in 777e25a
|
There should be no change in user-facing behavior, formatting. Just a change to the contents of the error object, which is currently never used IIUC. |
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.
Really happy with the simplification! Left a few minor comments inline.
src/shape.rs
Outdated
self.width | ||
.checked_sub(n) | ||
.map(|width| Shape { width, ..*self }) |
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.
Would it be better to define sub_width_opt
, and really all the *_opt
functions in terms of their Result<T, MaxWidthError>
versions?
pub(crate) fn sub_width_opt(&self, n: usize) -> Option<Shape> {
self.sub_width(n, rustc_span::DUMMY_SP).ok()
}
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 considered that and I opted to avoid using DUMMY_SP. But it's not a strong preference for me.
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.
Another possibility is to go the other way around with something like sub_width_opt(n).ok_or_else(|| self.max_width_error(span))
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 you're second idea would be a nice way to avoid using a DUMMY_SP
.
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.
done!
Avoid "width" since it can get confused with the width field.
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.
Thanks for helping simplify the code with these Shape
updates!
Shape
modifier methods now returnResult<Shape, ExeedsMaxWidthError>
_opt
variants of the methods for when an Option is still needed