-
Notifications
You must be signed in to change notification settings - Fork 14
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
[fontir] Unify Error & WorkError #855
Conversation
name: name.into(), | ||
kind: kind.into(), | ||
} | ||
} |
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.
Not loving the "smart" error, just construct it at the callsite with a glyph name and a kind, why do we need to be fancy?
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.
The rationale for this is that it cleans up the call site significantly, so when you're reading the code you can focus on the logic without getting too lost in the error handling.
Compare,
path_builder
.move_to((first.x, first.y))
.map_err(|e| BadGlyph {
name: glyph_name.clone(),
kind: BadGlyphKind::PathConversion(e),
})?;
add_to_path(&mut path_builder, contour.points[1..].iter()).map_err(|e| BadGlyph {
name: glyph_name.clone(),
kind: BadGlyphKind::PathConversion(e),
})?;
} else {
add_to_path(&mut path_builder, contour.points.iter()).map_err(|e| BadGlyph {
name: glyph_name.clone(),
kind: BadGlyphKind::PathConversion(e),
})?;
}
let path = path_builder
.build()
.map_err(|e| BadGlyph {
name: glyph_name.clone(),
kind: BadGlyphKind::PathConversion(e),
})?;
and,
path_builder
.move_to((first.x, first.y))
.map_err(|e| BadGlyph::new(glyph_name.clone(), e))?;
add_to_path(&mut path_builder, contour.points[1..].iter())
.map_err(|e| BadGlyph::new(glyph_name.clone(), e))?;
} else {
add_to_path(&mut path_builder, contour.points.iter())
.map_err(|e| BadGlyph::new(glyph_name.clone(), e))?;
}
let path = path_builder
.build()
.map_err(|e| BadGlyph::new(glyph_name.clone(), e))?;
Is there anything in particular you don't like about this, or is it just a pattern you aren't familiar with?
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.
If you dropped new and made BadGlyph a tuple-struct you would get equivalent readability, no? BadGlyph(glyph_name.clone(), e.into())
?
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.
In general I only use tuple structs when there's a single field, and that would also require these fields to be public, which they are not currently. Certainly this could be an exception, but it seems worse to me in identifiable ways. Maybe I'm overlooking some issue with new
? Are you worried about the costs of monomorphism? Is there another concern I've overlooked?
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.
LGTM although I might prefer if we dropped parse
- very strongly associated with actual parsing imho - and possibly also new
This also makes errors more granular: - errors related to finding, loading, or parsing a source file are their own 'BadSource' error type, which always preserves the path of the file in question; - errors that occur during conversion of a specific glyph are now their own 'BadGlyph' error type, which means we always preserve the name of the glyph in question when such an error occurs. A few other variants were removed or combined.
This also makes errors more granular:
A few other variants were removed or combined.