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

Add new Ratio output type --output-format=Ratio #167

Merged
merged 4 commits into from
Dec 25, 2020

Conversation

yijunyu
Copy link
Contributor

@yijunyu yijunyu commented Dec 20, 2020

I work on Huawei's Trusted Programming project, which was recently asked to report for Rust projects the "safe ratios", i.e. percentages of items that are safe. This could be done using cargo-geiger; however, currently it counts unsafe items, rather than reports the safety ratios,

I've completed a working prototype for this by adding --output-format=Ratio as an option to the command. If someone with more experience developing in cargo-geiger can give this an early look and send feedback, that would be really helpful.

Apologies in advance for the current lack of tests and limited documentation. I'll be working on this full time for a while, so it will improve rapidly.

Thanks!

@@ -22,6 +22,7 @@ pub enum OutputFormat {
Json,
GitHubMarkdown,
Utf8,
Ratio,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you order the OutputFormat types alphabetically here and in the updated help message please?

);
colorize(&status, output_format, output)
match output_format {
OutputFormat::Ratio => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to split out two functions here, one for OutputFormat::Ratio and one for _ with unit tests for each? It might be nice to have a function and tests for the ratio calculation too?

…atio and _ into two functions; adding a test case for regression
@yijunyu
Copy link
Contributor Author

yijunyu commented Dec 21, 2020

Thanks for the suggestions, just committed these changes: reorder OutputFormat alphabetically; separate OutputFormat::Ratio and _ into two functions; adding a test case for regression.

@anderejd
Copy link
Contributor

anderejd commented Dec 24, 2020

@jmcconnell26 Thanks for the review! I managed to miss this PR notice.
@yijunyu Thanks for the PR! This initiative makes sense to me, let's merge it if @jmcconnell26 is happy with the last commit.

@anderejd
Copy link
Contributor

Another perspective for future improvements: Perhaps we should have both --output-format and --metrics arguments to control the output? That way we can have ratios in for example the json report output format as well.

@yijunyu
Copy link
Contributor Author

yijunyu commented Dec 24, 2020

Another perspective for future improvements: Perhaps we should have both --output-format and --metrics arguments to control the output? That way we can have ratios in for example the json report output format as well.

That's a good suggestion, @anderejd. I will work on this after the PR is merged.

@anderejd
Copy link
Contributor

It looks like the review comments have been addressed, merging.

@anderejd anderejd merged commit 65cd410 into geiger-rs:master Dec 25, 2020
@jmcconnell26
Copy link
Contributor

@anderejd, apologies for the delay, yes the updated commits look good to me! Thanks @yijunyu

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.

3 participants