Skip to content
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

Improve error handling in fj_export #599

Merged
merged 3 commits into from
May 18, 2022

Conversation

chrisprice
Copy link
Contributor

Thanks for the feedback on #594, I'm always keen to learn. This PR attempts to address the comments. Please feel free to keep the comments flowing!

@chrisprice chrisprice requested a review from hannobraun as a code owner May 17, 2022 20:26
@chrisprice chrisprice force-pushed the support-emf-feedback branch from 6caf4de to 4dd2700 Compare May 17, 2022 21:26
hannobraun
hannobraun previously approved these changes May 18, 2022
Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the follow-up, @chrisprice!

This looks excellent! I left some comments with suggestions, but this is some high-level complaining I'm doing there. No need to make any changes, but of course, another follow-up PR is welcome, if you're so inclined.

}
None => Err(anyhow!("No extension specified")),
Some(extension) => Err(Error::InvalidExtension(
extension.to_str().map(|s| s.to_string()),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Option<String> in the error can make for a very confusing error message. How about changing it to String, and using something like this here instead:

Suggested change
extension.to_str().map(|s| s.to_string()),
extension.to_string_lossy().into_owned(),

#[error("no extension specified")]
NoExtension,

/// Unrecognised extension found `{0:?}`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy-pasting the error message works for the other doc comments, but not this one 😁

Suggested change
/// Unrecognised extension found `{0:?}`
/// Unrecognised extension found

@hannobraun hannobraun changed the title Fix STL PR feedback Add Triangle::normal / Fix STL PR feedback May 18, 2022
@hannobraun
Copy link
Owner

Hmm, I changed the PR title to highlight the addition of Triangle::normal, but on second thought, I'll pull that into a separate PR. Triangle::normal and fj_export::Error are both additions to the public API. But since they are in different crates, they need to be highlighted in different sections of the Weekly Dev Log and the release changelog.

This is not a complaint about this pull request, I'm just explaining what I'm doing. Feel free to split future pull requests accordingly, if you happen to remember, but otherwise don't worry. Keeping track of this organization stuff is my job 😁

@hannobraun hannobraun force-pushed the support-emf-feedback branch from 4dd2700 to c1ed3f0 Compare May 18, 2022 10:19
@hannobraun hannobraun changed the title Add Triangle::normal / Fix STL PR feedback Improve error handling in fj_export May 18, 2022
@hannobraun
Copy link
Owner

Rebased on top of #600. Merging.

@hannobraun hannobraun merged commit 983ed76 into hannobraun:main May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants