Suppress command exit code error messages #1354

gokhanettin · 2022-09-25T14:48:51Z

When a recipe wraps cli tool and the tool exits with a non-zero code,
just adds its own extra exit error message along with the messages
from the tool. Introduce no-exit-message attribute to suppress this
additional message.

Closes #1334

casey · 2022-09-28T05:16:36Z

I didn't take a close look, but this looks good. Thanks for writing tests! Let's figure out if this flag should also suppress command echoing.

casey · 2022-10-13T03:57:50Z

Nice, let me know when this is ready to review!

gokhanettin · 2022-10-14T19:54:33Z

@casey, I think it is ready to review. We can update the docs when we agree on the implementation.

I have a couple of concerns here. If we need to add multiple annotations, we are likely to support a syntax like [no-exit-message, annotation2, annotation3] which we can't represent with annotation: Option<Annotation<'src>> any more. It will probably be something like annotations: BTreeSet<Annotation<'src>>. This won't be backward compatible with the json representation. Do you think we should now have annotations: BTreeSet<Annotation<'src>> within the recipe struct even though we currently have only one annotation type?

casey · 2022-10-15T02:00:34Z

@casey, I think it is ready to review. We can update the docs when we agree on the implementation.

Nice! I'm just heading out of town for the weekend, so I won't be able to look at this until next week when I get back. Apologies for the delay.

I have a couple of concerns here. If we need to add multiple annotations, we are likely to support a syntax like [no-exit-message, annotation2, annotation3] which we can't represent with annotation: Option<Annotation<'src>> any more. It will probably be something like annotations: BTreeSet<Annotation<'src>>. This won't be backward compatible with the json representation. Do you think we should now have annotations: BTreeSet<Annotation<'src>> within the recipe struct even though we currently have only one annotation type?

Good thinking. One note: The JSON dump format is unstable, i.e., requires the --unstable flag, so it can be changed in backwards incompatible ways if necessary. Although I agree it's a good idea to avoid that if possible.

I actually think that recipes probably shouldn't have a BTreeSet of annotations, but rather they should have a suppress_exit_error_messages: bool field. My thinking is that by the time you have a Recipe struct, the actual annotations aren't important, just their semantics.

casey

This is looking good, thanks for all the tests!

A few high level thoughts:

I wasn't sure whether we should call these "attributes" or "annotations", so I did a google search for programming "function attribute" and programming "function annotation", and the search with "attribute" got 8x more hits, so I think we should go with that and call them "attributes" instead of annotations.
Since the json now just includes a single suppress_exit_error_messages boolean, which will be backwards compatible with future additions, I think we should avoid parsing the comma-separate attribute list syntax, i.e., [a,b,c], for now. We can easily add it in the future if we need it.
Since an attribute is required to be directly in front of a recipe, instead of popping an attribute off the list of items, in the main parser loop, we can parse an attribute if we see a [, and then immediatly parse a recipe:
```
} else if self.accepted(BracketL)? {
  items.push(Item::AnnotationContainer(self.parse_annotations()?));
  self.expect_eol()?;
  self.parse_recipe()?
}
```
This avoids the need to check for spurious newlines or make the pop_doc_comment function more complicated.
I think the signature of Parser::parse_atribute should be fn parse_attribute(&mut self) -> Result<Attribute>, and attribute can be:
```
enum Attribute {
  NoExitMessage
}
```
This is nice because the parser returns a single, strongly-typed attribute, so downstream code doesn't need to worry about checking for invalid attributes.

gokhanettin · 2022-10-20T16:59:09Z

Thanks for the review. I agree with all the thoughts. I will be back with the changes in a couple of days.

casey

Nice this is great! A few comments, but it's all small stuff.

README.md

src/attribute.rs

casey · 2022-10-23T00:06:05Z

src/compile_error_kind.rs

@@ -98,6 +98,9 @@ pub(crate) enum CompileErrorKind<'src> {
    alias: &'src str,
    target: &'src str,
  },
+  UnknownAttribute {
+    attribute: &'src str,


Suggested change

attribute: &'src str,

attribute: Name<'src>,

Other CompileErrorKinds like UnknownFunction, UnknownSetting all have &'src str. I thought there was nothing special about the attribute. Considering your parse_attribute_name suggestion, do you find anything wrong or ugly with the code below @casey?

... UnknownAttribute { attribute: &'src str, },

/// Parse a recipe attribute name fn parse_attribute_name(&mut self) -> CompileResult<'src, Attribute> { let name = self.parse_name()?; Attribute::from_name(name).ok_or_else(|| { name.error(CompileErrorKind::UnknownAttribute { attribute: name.lexeme(), }) }) }

I think we should have &'src str here. CompileError returned by error() method already has a token field. We should not have another Token or Name type field within the CompileErrorKind.

pub(crate) struct CompileError<'src> { pub(crate) token: Token<'src>, pub(crate) kind: Box<CompileErrorKind<'src>>, }

Great point, makes sense to me!

src/error.rs

casey · 2022-10-23T00:16:05Z

src/justfile.rs

@@ -472,6 +472,7 @@ mod tests {
      recipe,
      line_number,
      code,
+      ..


It's more verbose, but I think I'd extract and check these with an assert below:

Suggested change

..

suppress_error_message,

src/parser.rs

casey · 2022-10-23T00:31:03Z

src/parser.rs

+  /// Parse a recipe attribute
+  fn parse_attribute(&mut self) -> CompileResult<'src, Attribute> {
+    let token = self.expect(Identifier)?;
+    let lexeme = token.lexeme();
+    if let Some(attribute) = Attribute::from_lexeme(lexeme) {
+      self.expect(BracketR)?;
+      Ok(attribute)
+    } else {
+      Err(token.error(CompileErrorKind::UnknownAttribute { attribute: lexeme }))
+    }
+  }


This is not quite right, since you also need the token to produce the error message, but something like this:

Suggested change

/// Parse a recipe attribute

fn parse_attribute(&mut self) -> CompileResult<'src, Attribute> {

let token = self.expect(Identifier)?;

let lexeme = token.lexeme();

if let Some(attribute) = Attribute::from_lexeme(lexeme) {

self.expect(BracketR)?;

Ok(attribute)

} else {

Err(token.error(CompileErrorKind::UnknownAttribute { attribute: lexeme }))

}

}

/// Parse a recipe attribute name

fn parse_attribute_name(&mut self) -> CompileResult<'src, Attribute> {

let name = self.parse_name()?;

Attribute::from_lexeme(name).ok_or_else(|| token.error(CompileErrorKind::UnknownAttribute { attribute: lexeme }))

}

gokhanettin · 2022-10-23T23:10:58Z

I am not sure about CompileErrorKind::UnknownAttribute field type. Pushed all other suggestions. Also need to update GRAMMER.md.

Is this OK?

recipe        : attribute? '@'? NAME parameter* variadic? ':' dependency* body?

attribute     : '[' NAME ']' eol

casey

One minor comment, otherwise looks great. Your suggestion for representing attributes in the grammar looks good to me.

casey · 2022-10-25T05:09:43Z

src/parser.rs

-        items.push(Item::Recipe(self.parse_recipe(doc, true)?));
+        items.push(Item::Recipe(self.parse_recipe(doc, true, false)?));
+      } else if self.accepted(BracketL)? {
+        self.parse_attribute_name()?;


This works, but it's a bit unclear what's going on. I'd do something like:

let Attribute::NoExitMessage = self.parse_atribute_name()?;

This makes it clear what's going on, and it will turn into a compile error when we add another variant.

When a recipe wraps cli tool and the tool exits with a non-zero code, just adds its own extra exit error message along with the messages from the tool. Introduce `no-exit-message` attribute to suppress this additional message.

casey · 2022-10-25T23:34:21Z

Very nice! Great work, especially for a non-trivial PR that touches the parser!

gokhanettin mentioned this pull request Sep 25, 2022

Supress the "error: Recipe ... failed on line ... with exit code ..." without suppressing the exit code #1334

Closed

gokhanettin force-pushed the suppress-command-exit-code-error-messages branch from 80f07eb to 504c354 Compare October 14, 2022 19:38

casey requested changes Oct 20, 2022

View reviewed changes

gokhanettin force-pushed the suppress-command-exit-code-error-messages branch from 96f4ba0 to a54b8d7 Compare October 21, 2022 22:49

gokhanettin marked this pull request as ready for review October 21, 2022 22:50

gokhanettin requested a review from casey October 21, 2022 22:51

gokhanettin force-pushed the suppress-command-exit-code-error-messages branch 2 times, most recently from 815dc97 to fd9ba23 Compare October 22, 2022 13:08

casey requested changes Oct 23, 2022

View reviewed changes

gokhanettin force-pushed the suppress-command-exit-code-error-messages branch from fd9ba23 to cbb4a81 Compare October 23, 2022 22:55

gokhanettin requested a review from casey October 23, 2022 23:11

casey requested changes Oct 25, 2022

View reviewed changes

Add a recipe attribute to suppress exit messages

ef0278a

When a recipe wraps cli tool and the tool exits with a non-zero code, just adds its own extra exit error message along with the messages from the tool. Introduce `no-exit-message` attribute to suppress this additional message.

gokhanettin force-pushed the suppress-command-exit-code-error-messages branch from 19a4f1f to ef0278a Compare October 25, 2022 19:09

gokhanettin requested a review from casey October 25, 2022 19:13

casey added 3 commits October 25, 2022 16:22

Merge branch 'master' into suppress-command-exit-code-error-messages

208910b

Fix for rust 1.56

a76cc2f

Placate clippy

f907b8e

casey merged commit 8b7640b into casey:master Oct 25, 2022

casey mentioned this pull request Oct 25, 2022

Add extensible recipe and justfile annotations #604

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppress command exit code error messages #1354

Suppress command exit code error messages #1354

gokhanettin commented Sep 25, 2022 •

edited

Loading

casey commented Sep 28, 2022

casey commented Oct 13, 2022

gokhanettin commented Oct 14, 2022

casey commented Oct 15, 2022 •

edited

Loading

casey left a comment

gokhanettin commented Oct 20, 2022

casey left a comment

casey Oct 23, 2022

gokhanettin Oct 23, 2022

casey Oct 23, 2022

casey Oct 23, 2022

casey Oct 23, 2022

gokhanettin commented Oct 23, 2022

casey left a comment

casey Oct 25, 2022

casey commented Oct 25, 2022

Suppress command exit code error messages #1354

Suppress command exit code error messages #1354

Conversation

gokhanettin commented Sep 25, 2022 • edited Loading

casey commented Sep 28, 2022

casey commented Oct 13, 2022

gokhanettin commented Oct 14, 2022

casey commented Oct 15, 2022 • edited Loading

casey left a comment

Choose a reason for hiding this comment

gokhanettin commented Oct 20, 2022

casey left a comment

Choose a reason for hiding this comment

casey Oct 23, 2022

Choose a reason for hiding this comment

gokhanettin Oct 23, 2022

Choose a reason for hiding this comment

casey Oct 23, 2022

Choose a reason for hiding this comment

casey Oct 23, 2022

Choose a reason for hiding this comment

casey Oct 23, 2022

Choose a reason for hiding this comment

gokhanettin commented Oct 23, 2022

casey left a comment

Choose a reason for hiding this comment

casey Oct 25, 2022

Choose a reason for hiding this comment

casey commented Oct 25, 2022

gokhanettin commented Sep 25, 2022 •

edited

Loading

casey commented Oct 15, 2022 •

edited

Loading