Add echo-comments setting

[neunenak]

Add a new setting "ignore-recipe-comments". If set, this causes lines internal to a non-shebang recipe beginning with the character '#' (other than '#!' itself) to be treated as comments of the justfile itself. They will not be echoed to stderr when the recipe executes.

Can potentially close #1274

[neunenak]

I'm not 100% sure if "ignore-recipe-comments" is the best possible name for what this setting does. It's more like, don't echo comments within a recipe, but that's a bit unwieldy to make for a setting name.

[erikkrieg]

This is a nice refactor, imo, but I think you could take it one more step if you wanted and turn this into a match expression instead of a series of if/else.

Just looking around out of curiosity and going to throw my 2 cents in (feel free to take it or leave it).

[neunenak]

I originally thought it wasn't possible to do this because lexeme is &str and can't be directly pattern-matched against Keyword, but I think if I parse the lexeme first I can pattern-match against that and make this bit of code a bit more concise. Thanks for the suggestion!

[casey]

What about set echo-comments := false?

[casey]

Wondering if maybe set comment-regex := "#.*" is potentially a better solution, since otherwise what is a comment is hard coded.

[neunenak]

Wondering if maybe set comment-regex := "#.*" is potentially a better solution, since otherwise what is a comment is hard coded.

I thought about that, but I think that hard-coding the notion of a Justfile comment does make sense.

There's basically two types of recipes, shebang and linewise. A linewise recipe is lines of shell code, and just does some additional interpretation of those lines as it executes - printing them before execution, paying attention to the @ sigil, etc. It makes sense that comments here should also be interpreted by just, and the most natural thing to do with comments interpreted by just within the body of a recipe is treat them exactly the same as comments outside the body of a recipe, i.e. ignore them because they are intended for the human reader of a justfile. And it makes sense that the syntax for a comment - namely, a line beginning with # - should be the same everywhere in a justfile that just directly interprets (ignoring for the time being the question of possibly having separate syntax for doc comments). So there's no need for a special command to specify what a comment looks like, it's just always #.

In a shebang recipe, the entire recipe block is treated as source code in some other language, and just doesn't try to interpret anything about the text. It's already the case that in a recipe like:

some-recipe:
    #!/usr/bin/node

    // Comments
    console.log("hello")

just doesn't try to interpret the // as a comment - it doesn't know or care that Javascript uses // for comments. This is exactly equivalent to:

some-recipe:
    #!/usr/bin/python

    # Comment
    print("hello")

where just doesn't know or care that Python happens to use the same #-convention for comments as just itself. And in the node case, something like:

some-recipe:
    #!/usr/bin/node

    # comments
    // Comments
    console.log("hello")

Currently fails at the node level rather than at the just level, because just doesn't care that # isn't a valid Javascript comment and node is doing exactly what it's supposed to do in the face of invalid Javascript syntax.

This PR as written only applies to linewise recipes and not shebang ones (maybe it would make sense to clarify that in the definition of the setting), so it basically establishes that a just comment is # everywhere except in a shebang recipe, where just simply doesn't allow its own comments. I think that's a sensible convention.

[neunenak]

What about set echo-comments := false?

The only issue there is that it means that the option would have to default to true in order to retain the current behavior in existing justfiles. All the other boolean options are default false. Is that a principled decision or just a coincidence?

[casey]

Sorry for letting this sit!

My concern is that someone might use set shell to change their linewise interpreter to something that doesn't use # as a comment, and then it would look wierd to have # comments in a language that didn't support it.

But thinking about it, it seems like having a simple setting that does the right thing 99.9% of the time is probably a good idea, and if someone complains that their justfile looks weird, we can cross that bridge when we come to it.

Is that a principled decision or just a coincidence?

It's kind of a principled decision, since I think it's nice when settings are default off, but if we don't have a better name, I'm open to breaking it.

I think echo-comments := false is probably better than ignore-recipe-comments := true, because it's not entirely clear what ignoring a comment means. I.e. what would it do with a comment if it's not ignoring it?

[casey]

If this is only called for linewise recipes, then does it need to check for #!?

[neunenak]

It doesn't, but the code can't guarantee that no one will ever try to call is_comment in a shebang recipe in the future.

On the other hand, maybe it makes sense to say that, once you're in the body of a linewise recipe, #! should be interpreted as a comment that happens to start with a ! rather than a shebang. Actually I think this is the correct thing to do, because right now just only checks to see if the first line in a recipe is a #!, not any other ones. So if the code encounters a #! after the first line, it should treat it as a comment that happens to begin with !, not as a shebang.

[neunenak]

It's kind of a principled decision, since I think it's nice when settings are default off, but if we don't have a better name, I'm open to breaking it.

I think echo-comments := false is probably better than ignore-recipe-comments := true, because it's not entirely clear what ignoring a comment means. I.e. what would it do with a comment if it's not ignoring it?

How about suppress-comment-echo, which can default to false?

Edit: Thinking about this again, echo-comments is a more succinct name for the option, and I think it's not actually a bad thing to have some options as default true and others as default false, as long as this is clearly marked in the documentation. So my vote would be for echo-comments defaulting to True, and break the default is always False convention.

[neunenak]

Btw @casey do you know why https://github.com/casey/just/actions/runs/3120617565/jobs/5061331667 is failing? It looks like this is because of clippy errors, but when I run stable clippy locally it reports nothing wrong.

[casey]

@neunenak Sounds good, let's go with echo-comments.

Try with rust 1.56, which is what CI uses: rustup install 1.56 && cargo +1.56.0 clippy --all --all-targets

[casey]

Nice, looks good to me!

[casey]

Whoops! I actually found something weird. Line continuations in comments cause the next line to be included in the comment, so this test fails:

#[test]
fn continuations() {
  Test::new()
    .justfile(
      "
      set echo-comments := false

      some_recipe:
        # Alternate shells still ignore comments \\
        echo something-useful
    ",
    )
    .stdout("something-useful\n")
    .stderr("echo something-useful\n")
    .run();
}

This is a bit odd, since normally the contents of a comment, including backslashes, are ignored. What do you think?

This also made me think of potentially better name for this setting skip-comments, since not only does it not echo them, it also doesn't invoke the shell with them. The setting could also default to false. Not sure though, since not echoing comments is the main effect, and maybe echo-comments := false is clearer than skip-comments := true.

[casey]

And sorry for letting this sit! Next time hit the request review button or add a comment letting me know it's ready to take a look at.

[neunenak]

And sorry for letting this sit! Next time hit the request review button or add a comment letting me know it's ready to take a look at.

No worries. I'm actually not sure where in the Github UI the request review button is (I've been using Gitlab at work so I'm much more used to their UI these days). As I recall, Github had a notion of Draft vs Ready open PRs, but I'm not sure where a "ready to review" button would live, unless that's a project-specific thing.

[neunenak]

Whoops! I actually found something weird. Line continuations in comments cause the next line to be included in the comment, so this test fails:

#[test]
fn continuations() {
  Test::new()
    .justfile(
      "
      set echo-comments := false

      some_recipe:
        # Alternate shells still ignore comments \\
        echo something-useful
    ",
    )
    .stdout("something-useful\n")
    .stderr("echo something-useful\n")
    .run();
}

This is a bit odd, since normally the contents of a comment, including backslashes, are ignored. What do you think?

This also made me think of potentially better name for this setting skip-comments, since not only does it not echo them, it also doesn't invoke the shell with them. The setting could also default to false. Not sure though, since not echoing comments is the main effect, and maybe echo-comments := false is clearer than skip-comments := true.

Huh, this is weird, and not something I expected - probably has something to do with how the tokenizer works? I would definitely expect that test case to succeed, I'll try to fix it.

[neunenak]

Actually, thinking about this a bit, I think this is a bug (or at least unintuitive behavior) with how just comments work in general. Ignoring this change, on the current published version of just, a justfile:

some-recipe:
    # a comment \\
    echo "HELLO"

will print the following:

➤ just some-recipe
# a comment \echo "HELLO"

and not print "HELLO".

This is definitely unintuitive, but it's also how just comments currently work, and it's the sort of thing that would definitely break existing justfiles if it changed suddenly. Deciding what to do about this deserves it's own issue (I'd suggest adding this to the list of things to fix in just edition 2). I don't think it needs to block this PR though, since echoing or not echoing what just treats as a comment is orthogonal to changing what just treats as a comment in the presence of line continuations.

[casey]

So I think the issue is that, in recipe bodies, just doesn't have comments at all. It doesn't do anything differently if a line starts with #.

This change makes just treat those lines as comments, so any aspect of how they behave can be changed, and it wouldn't be a breaking change, since it's opt-in with the setting. I think it's reasonable to have this setting just kind of turn on comments in general, i.e. doesn't echo them, and also considers them to be a single line, and ignores line continuations at the end of them.

[neunenak]

So I think the issue is that, in recipe bodies, just doesn't have comments at all. It doesn't do anything differently if a line starts with #.

This change makes just treat those lines as comments, so any aspect of how they behave can be changed, and it wouldn't be a breaking change, since it's opt-in with the setting. I think it's reasonable to have this setting just kind of turn on comments in general, i.e. doesn't echo them, and also considers them to be a single line, and ignores line continuations at the end of them.

Makes sense. This actually turned out to be pretty easy to fix to make that test case pass - just add if line.is_continuation() && !comment_line { ... } to the appropriate spot in the loop in run_linewise in recipe.rs.

[casey]

Nice, merged! I decided to change the name back to ignore-comments. I think it's the clearest. Thanks for sticking with this! I expect that this is something that we'll want to default to true in a future edition.