Add --fmt subcommand

[vglfr]

Implements #817.

Things I'd like to get feedback so I could fix them:

• copypasted impl<'src> Display for UnresolvedRecipe<'src> (cannot wrestle borrow checker)
• ugly tree: (justfile ()) instead of tree: (justfile) to pass tests (not sure that matters)
• justfile in repository uses tabs but --fmt formats them to spaces, probably because it reuses space-based impl originally used for --dump (I don't have any preference on the matter)

[casey]

Thanks for taking a crack at this:

• copypasted impl<'src> Display for UnresolvedRecipe<'src> (cannot wrestle borrow checker)

What was the issue that you ran into? I think you could bound D in Recipe by Display:

pub(crate) struct Recipe<'src, D: Display = Dependency<'src>> {

You'll also need to add it to the impl blocks:

impl<'src, D: Display> Recipe<'src, D> {

• ugly tree: (justfile ()) instead of tree: (justfile) to pass tests (not sure that matters)

If there are no comments, shouldn't that avoid an empty ()?

• justfile in repository uses tabs but --fmt formats them to spaces, probably because it reuses space-based impl originally used for --dump (I don't have any preference on the matter)

I'm not sure what --fmt should use by default. I use tabs, because for a long time I used make-based syntax highlighting in vim, which highlights space-indentation as an error, since makefiles only use tabs.

I used to dislike tabs, but have actually come around to them, since tabs let everyone use a different tab-width. I'm inclined to keep them, and change --dump to use tabs.

[casey]

Actually, for now, let's use spaces, just to keep the PR-size reasonable. We can adjust --fmt defaults and configuration going forward.

[vglfr]

Fixed it, thanks! It turns out impl<'src> Display for Recipe<'src> needs to be replaced by impl<'src, D: Display> Display for Recipe<'src, D>.

As for (justfile ()) it's probably false alarm. In that particular case justfile.tree() is called on # foo. Can't see any harm done, it's just untidy.

[casey]

Let me know when this is ready to review. The main thing I'd like to see are a bunch of tests, probably for every construct, that the output of --fmt is reasonable, and also a valid justfile. I think the output of --dump should be the same as --fmt, so the tests can call --dump and check the output. See the tests in tests/misc.rs for some examples. The new tests can go in tests/fmt.rs.

[vglfr]

I think it's ready. Few random notes before review:

• --fmt and --dump are the same for recipes, assignments, and aliases; --fmt additionally supports sets and comments
• --fmt removes extra newlines between two aliases, sets, assignments, or comments, thus implementing basic grouping
• conditional inside recipe changed from {{if ... { } else { } }} to {{ if ... { } else { } }} while dropping trailing whitespace
• indented backticks keep their triple quotes (previously were formatted to single quote)

[casey]

Awesome! I just ran CI, and I think the completion scripts are out of date.

• --fmt and --dump are the same for recipes, assignments, and aliases; --fmt additionally supports sets and comments

I think that --dump and --fmt should produce the same output, so I would leave comments in --dump. I looked at the code, but it wasn't obvious to me why sets would be different between the two. Are they handled differently?

• --fmt removes extra newlines between two aliases, sets, assignments, or comments, thus implementing basic grouping

Sounds good!

• conditional inside recipe changed from {{if ... { } else { } }} to {{ if ... { } else { } }} while dropping trailing whitespace

Also good, it definitely looks better this way.

• indented backticks keep their triple quotes (previously were formatted to single quote)

Nice. We might eventually want to make --fmt convert indented backticks to some standard formatting, but this is great for now.

[vglfr]

Updated completions.

I think that --dump and --fmt should produce the same output, so I would leave comments in --dump. I looked at the code, but it wasn't obvious to me why sets would be different between the two. Are they handled differently?

Yes, --dump isn't printing sets at all. I think that's original behavior. Adding sets to --dump should be straightforward. However it might be impossible to determine from Justfile struct whether setting was set explicitly (when it is default). So either all settings or non-default settings could be dumped. --fmt doesn't have this dilemma, it just formats all sets as they appear in justfile.

Recipe docstring comments are displayed in --dump all right, as well as comments in recipe body. However non-recipe-docstring top-level comments are skipped (including essential ones like first line shebang). Adding them to --dump would require modifying Justfile struct.

Please let me know what's the right way to handle those.

[casey]

Ah, right. That makes sense. I think we should change it so that --dump prints out the Module, like --fmt. I don't think anyone is relying on comments being stripped from --dump, and making them do the same thing would allow users to, for example, redirect the output of --dump to a new justfile and have it be valid.

[vglfr]

Updated. Redirecting --dump sounds cool.

[casey]

Awesome! Just pushed a commit fixing a small formatting diff. I'll check this out today or tomorrow.

[casey]

This looks great! Some minor comments.

[vglfr]

Fixed. Thanks for a review!

[casey]

Looks good! A few minor things.

[casey]

I think that adding dotenv-load := true to tests was actually just a temporary hack to make refactoring easier. I removed it in #853, so I think all the special case stuff around it can be removed. Sorry for the churn! I should have removed it long ago.

[vglfr]

Fixed, thanks!

[casey]

Hooookay!

Sorry for the delay in reviewing this. I was in Miami for the Bitcoin conference. (Which was like a Bitcoin-themed circus, a bit superficial, but lots of fun.)

I changed the help message text a little bit, refactored the format subcommand to make Display write directly to disk instead of creating a temporary, and refactored the doc comment handling a little bit.

My concern with the doc comment handling was that doc comment might get "lost", i.e. never actually turned into an item. I changed it so that comments are always inserted into the items vector, and popped off and passed into each call of parse_recipe. (Only if eol_since_last_comment is false, which is set to true whenever the parser encounters a newline.)

I was going to say that this is ready to merge, but I thought of something looking over the code one last time. Comments that are on the same line as items aren't preserved. For example, this comment won't be preserved as an Item.

alias x := y # foo

Since this PR is already pretty large, I was thinking about merging this, and then adding an --unstable flag. Unless --unstable was passed, --fmt would return an error. My thinking is that it's okay if the output of --dump is missing things, but since --fmt overwrites the justfile, there's a higher bar for preserving everything in the justfile.

What do you think?

[casey]

Just thinking out loud:

The method that swallows comments is expect_eol. I'm thinking that we should move the items vec into the Parser struct, and then have individual parsing functions push items into the vec, instead of returning items. This would allow methods like expect_eol to insert comments. I'm not sure the best way preserve the position of comments (whether they appear on lines by themselves or after items). We might have to create an Item::Eol that's created for each newline.

[vglfr]

Hi, thanks for fixes, they're really instructive (which is what I'm looking for - besides the fact that I'm using Just in all my projects and would love to pay back).

Same line comments is something I've overlooked. Totally fine with --unstable.

[casey]

Hi, thanks for fixes, they're really instructive (which is what I'm looking for - besides the fact that I'm using Just in all my projects and would love to pay back).

You bet!

Same line comments is something I've overlooked. Totally fine with --unstable.

Awesome, I'll merge this now, and add an --unstable flag before the next release.

[casey]

Merged! Thanks again for the initiative in tacking this!