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

Fix panic when invoked on crate missing metadata (#377) #378

Merged
merged 1 commit into from
Feb 21, 2021

Conversation

gkelly
Copy link
Contributor

@gkelly gkelly commented Feb 21, 2021

Also add a test that ensures the error is reported and a panic doesn't
occur.

@gkelly gkelly requested a review from acmcarther as a code owner February 21, 2021 22:13
@gkelly
Copy link
Contributor Author

gkelly commented Feb 21, 2021

With this patch instead of panicking the result is now:

Error: Raze failed with cause: "Exactly one package must contain primary raze settings: []"

Copy link
Collaborator

@dfreese dfreese left a comment

Choose a reason for hiding this comment

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

Thanks for the help providing clearer errors.

@@ -626,9 +626,9 @@ fn parse_raze_settings_any_package(metadata: &Metadata) -> Result<RazeSettings>
}

// There should only be one package with raze
if settings_packages.len() > 1 {
if settings_packages.len() != 1 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's probably worth specifying the zero case separately. Perhaps something like:

No raze settings were specified in the Cargo.toml file, see README.md for details on expected fields

would be more helpful?

also, not your doing, but I'm noticing that

return Err(anyhow!("error message"));

is used. Anyhow provides anyhow::bail

bail!("error message");

that does the same thing, more concisely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's probably worth specifying the zero case separately. Perhaps something like:

No raze settings were specified in the Cargo.toml file, see README.md for details on expected fields

would be more helpful?

Done. I do worry that it should perhaps be a URL, as if someone is just running cargo-raze as part of CI, etc, that it won't be clear which README.md is intended in context.

also, not your doing, but I'm noticing that

return Err(anyhow!("error message"));

is used. Anyhow provides anyhow::bail

bail!("error message");

that does the same thing, more concisely.

Done.

Also add a test that ensures the error is reported and a panic doesn't
occur.
@gkelly gkelly force-pushed the fix-panic-without-metadata branch from c6a0c12 to dbe5c0b Compare February 21, 2021 22:46
@gkelly gkelly requested a review from dfreese February 21, 2021 22:49
@dfreese dfreese merged commit 25bcadb into google:master Feb 21, 2021
@dfreese
Copy link
Collaborator

dfreese commented Feb 21, 2021

great, thanks!

@gkelly
Copy link
Contributor Author

gkelly commented Feb 21, 2021

great, thanks!

I appreciate the reviews and merge, thanks!

@gkelly gkelly deleted the fix-panic-without-metadata branch February 21, 2021 23:01
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