-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
Print message to help user manage false positives #80
Conversation
Thanks, that's a nice touch. Do you think it'd make sense to publicize the |
Sure, that makes sense to me. When user doesn't pass
When user does pass
|
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.
Nice! I think we can avoid a bit of text duplication, and this should be good to go 👍
src/main.rs
Outdated
@@ -211,6 +211,33 @@ fn run_machete() -> anyhow::Result<bool> { | |||
} | |||
} | |||
|
|||
if has_unused_dependencies { | |||
if !args.with_metadata { | |||
println!("\n\ |
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.
nit: we could use a raw string here:
println!(r#"my
long
string");
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 tried, but the formatting was very unpleasant with a raw string, especially after cargo fmt got its hands on it.
src/main.rs
Outdated
); | ||
} else { | ||
println!("\n\ | ||
If you believe cargo-machete has detected an unused dependency incorrectly,\n\ |
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.
Could we use const
ant str to avoid the code duplication here? This would also help spot that we're showing almost the same text, and that would highlight what the differences are too.
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 gave something like that a shot, but the result reads "inside-out", which is awkward:
if has_unused_dependencies {
let middle = if !args.with_metadata {
"you can try running it with the `--with-metadata` flag for better accuracy,\n\
though this may modify your Cargo.lock files.\n\
\n\
You can also add the dependency to the list of dependencies to ignore in the\n"
} else {
"you can add the dependency to the list of dependencies to ignore in the\n"
};
println!(
"\n\
If you believe cargo-machete has detected an unused dependency incorrectly,\n\
{middle}\
`[package.metadata.cargo-machete]` section of the appropriate Cargo.toml.\n\
For example:\n\
\n\
[package.metadata.cargo-machete]\n\
ignored = [\"prost\"]"
);
}
Maybe I'm missing an obvious way to make this less contorted.
In the end, I moved the part which is only applicable sometimes -- the --with-metadata
suggestion -- to the end. That simplifies things, though it obviously means the --with-metadata
part is mentioned second, which I originally didn't like as much. But it seems fine. Anyway, let me know what you prefer here.
After reducing some duplication, without
With
|
5587d56
to
1e5c9b8
Compare
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.
Nice, thank you!
When unused dependencies are found, print a message that tells the user how to manage false positives.
Example output: