-
Notifications
You must be signed in to change notification settings - Fork 476
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 recipe signature if missing arguments #369
Conversation
references issue #341
Thank you for the PR! I think this has a stray 2nd commit. You might want to create a separate branch for the pull request, so that pushes to master don't land on the PR after you've opened it. |
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.
Parameters have a Display
implementation, so that should be reused to format optional and
variadic parameters, and to color the usage string. I use the {:#}
formatter for colored output. Since this message is being printed to stderr, it should use {:#}
of stderr is attached to a terminal, and {}
otherwise. Check out run.rs
for how to do this.
D'oh! That actually makes sense, I'm just so used to not using branches at work (company policy...) so I forget sometimes to use them lol Did you want the usage printed on all of the conditions in |
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.
Sweet, thanks for making those changes. We should check stderr instead of stdout, and I think we should print the usage string in all cases. After that this should be good to go.
src/runtime_error.rs
Outdated
@@ -79,7 +79,11 @@ impl<'a> Display for RuntimeError<'a> { | |||
if expected < found { "only " } else { "" }, expected)?; | |||
write!(f, "usage:\n just {}", recipe)?; | |||
for param in parameters { | |||
write!(f, " {:#}", param)?; | |||
if color.stdout().active() { |
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.
Error messages are written to stderr, so we should check that instead of stdout.
src/justfile.rs
Outdated
@@ -99,6 +99,7 @@ impl<'a> Justfile<'a> where { | |||
if !argument_range.range_contains(argument_count) { | |||
return Err(RuntimeError::ArgumentCountMismatch { | |||
recipe: recipe.name, | |||
parameters: recipe.parameters.iter().collect::<Vec<_>>(), |
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.
You can actually leave out the type hint to collect
. Since it's being assigned to ArgumentCountMismatch::parameters
, the rust compiler can infer that the result must be a vec.
Awesome, thank for your contribution! |
Thank you! Happy to contribute 💙 |
references issue #341
Wasn't sure what format would be the best, but I took a stab at it and and printed out a simple usage doc on top of the error that was already there. Let me know what more I can do to help/make the code cleaner! I'm still fairly new to rust, so if this seems like a naÏve approach, please forgive me 😆