-
Notifications
You must be signed in to change notification settings - Fork 510
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
Adds no-exit-message
Setting and [exit-message]
attribute
#2568
Conversation
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.
Looks good!
- I think there is an additional test case, just a plain recipe with the
[no-exit-message]
attribute with no setting. -
[exit-message]
and[no-exit-message]
on the same recipe should produce an error, and this should also have a test
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.
All comments done.
Added additional suggested tests and moved all tests to no_exit_messsage.rs
.
Also added a commit to refactor the other tests in no_exit_messsage.rs
to Test::new()
if that is suitable for this PR.
Let me know if there is anything else that needs changing.
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.
Looks good! I merged latest master in, and changed the order of tests so they're easier to review.
Only one thing left, having [exit-message]
and [no-exit-message]
on the same recipe should be a compiler error.
You can see an example with the working-directory
attribute and no-cd
attribute in parser.rs:
let working_directory = attributes.contains(AttributeDiscriminant::WorkingDirectory);
if working_directory && attributes.contains(AttributeDiscriminant::NoCd) {
return Err(
name.error(CompileErrorKind::NoCdAndWorkingDirectoryAttribute {
recipe: name.lexeme(),
}),
);
}
You'll have to create a new CompileErrorKind
variant.
@casey added the requested changes, thank you for reviewing and for the feedback! |
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.
LGTM!
This adds both the
no-exit-message
setting and the[exit-message]
attribute. Current behaviour is[exit-message]
overrides bothset no-exit-message
and[no-exit-message]
.Idea from #2566.
I would like feedback on whether I need to add anything to the documents and whether any test cases are missing.