Skip to content
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

Allow listing recipes in submodules with --list-submodules #2113

Merged
merged 4 commits into from
May 30, 2024

Conversation

casey
Copy link
Owner

@casey casey commented May 30, 2024

@crdx Do you prefer a nested view, like this:

Available recipes:
  foo
  bar

  abc:
    xyz
    wer

  hjk:
    yui
    asd

Or a flattened view, like this:

Available recipes:
  foo
  bar
  abc::xyz
  abc::wer
  hjk::yui
  hjk::asd

@casey casey mentioned this pull request May 30, 2024
2 tasks
@valscion
Copy link

valscion commented May 30, 2024

If I may add my preference? I'm in favor of the nested view as I'm not that comfortable showing :: anywhere 😅

The way we've used the "module-like" just scripts so far has relied on the separator being a space character and I'd like to avoid seeing the :: in any output 😅

I can see us using --list-submodules in some inner module, maybe already with the just devices case I pointed in #929 (comment) — but not at the top-level just case.

The :: separator feels like some Rust style preference to me and not something I've seen in any other command line tool we use.

The old way of using "module-like" just scripts has also relied only on whitespace separated modules so we don't have any usage of e.g. just devices::android::open-url in the past but just devices android open-url instead.

I do agree that it's slightly easier to read the output when the list contains of abc::xyz as it's shorter — but I don't think there would be too many submodules to list.


One more question: Should this have some sort of depth limit option as well? What if there are submodules on 4 levels — should all of them be shown or would it be possible to limit the depth somewhere? I don't know if that would be useful to have but I bet it would be asked at some point in the future 😅

@casey
Copy link
Owner Author

casey commented May 30, 2024

The issue with spaces in a flattened view is that it wouldn't be possible to tell what was a submodule/recipe and what was an argument:

Available recipes:
  foo
  bar
  abc xyz
  abc wer
  hjk yui
  hjk asd

abc could be a recipe with an argument named xyz. The ::-separated notation is also necessary in few places. I chose it because :: is commonly used to separate module paths, e.g., in Rust, C++, and I think others.

The output of --summary uses spaces to separate recipe names, and completion scripts split on spaces, so in order to include recipes in submodules, they must be ::-separated. Because of this, :: must also be accepted on the command line, so that a recipe from the output of --summary can be passed to just.

When, eventually, you can reference variables and recipes in other modules, you'll have to specify module paths using ::. This is just because the grammar would otherwise be ambiguous between submodule recipes and arguments:

mod foo

bar: foo::baz

So the command-line interfaces will all accept :: as an alternative to spaces, and when module paths eventually appear in justfiles themselves, they'll be space-separated. I do agree that spaces are nicer!

I'll probably add some sort of option to list submodules nested, as in this PR, and flattened, in another PR, using ::.

As far as a depth limit, I'd be happy to add that in the future, feel free to make an issue if/when you want it!

@casey casey marked this pull request as draft May 30, 2024 08:07
@valscion
Copy link

valscion commented May 30, 2024

Yeah to clarify my opinion — I don't have any objections of using :: inside Justfiles themselves. I do think it makes a lot of sense to use :: inside Justfiles.

I'm mostly against showing that in the output of any just commands when ran on the command line. (At least those which are intended to be used regularly and not for mostly debugging purposes)

I also do agree that the flattened view as in your example would be confusing and would not want that either.

It is indeed a tricky case!

The output of --summary uses spaces to separate recipe names, and completion scripts split on spaces, so in order to include recipes in submodules, they must be ::-separated. Because of this, :: must also be accepted on the command line, so that a recipe from the output of --summary can be passed to just.

I'm not sure I fully follow this train of thought, so let me try to understand what you mean with this example:

Available recipes:
  foo
  bar

  abc:
    xyz
    wer

  hjk:
    yui
    asd

I would image just <TAB> would show in autocompletion:

foo
bar
abc
hjk

And then I'd do just abc <TAB> and see:

xyz
wer

But it sounded like what I'd have instead is this when running just <TAB>?

foo
bar
abc::xyz
abc::wer
hjk::yui
hjk::asd

Do correct me if I'm wrong! I'm mostly worried that the tab autocompletions would get useless for us if all the submodule recipes would also be shown in there with the :: separating them instead of needing "diving deeper" like we now have since #2108

@crdx
Copy link
Contributor

crdx commented May 30, 2024

@casey The original nested output is my personal preference. I wouldn't use the flattened view.

I don't use the concept of a default recipe, i.e. I don't have just on its own run anything. Instead, the first recipe is always:

[private]
help:
    just --list --unsorted

Because I prefer to see what's available and pick from the list rather than have a default. So, I can easily add a new flag --nested (or whatever) to this recipe everywhere.

@valscion I believe that the output of --summary is correct to show fully-qualified recipes. Technically a submodule recipe is still a recipe that can be run from the top-level context so completions should show it. And it needs to be ::-separated because of completion word-splitting characters.

The current shell completions are generated by applying simple replacements to the clap-generated default completions:

https://github.com/casey/just/blob/f5bb82dea3edde07b5aca4303e2d73a300f45d5e/src/completions.rs

I wouldn't personally like to try to implement more intelligent submodule recipe completion handling like this. This is the point where hand-crafted completion scripts would be better.

@valscion
Copy link

Technically a submodule recipe is still a recipe that can be run from the top-level context so completions should show it.

Yeah I do agree — but this "technically correct" vs. what I'm in favor of as "intuitive behavior" are different.

For us, it would make the autocompletion cumbersome and the first tab hit after entering just <TAB> would be useless as we'd see hundreds of potential autocompletions. At that point, we would not see the rest of the commands available at all.

For example, the aws CLI (which is huge!) also doesn't show "submodules" but instead lists the outermost modules first:

image

Then I can tab to just the aws s3 scripts:

image

This is how I'd expect "top level modules" to first work and then submodules showing only after I've explicitly tabbed to show the submodules.

@casey casey marked this pull request as ready for review May 30, 2024 17:25
@casey casey merged commit d38c1ad into master May 30, 2024
5 checks passed
@casey casey deleted the list-submodules branch May 30, 2024 17:28
@casey
Copy link
Owner Author

casey commented May 30, 2024

The original nested output is my personal preference. I wouldn't use the flattened view.

Okay, nice. I'm going to merge this, if someone wants the flat view, they can open an issue.

The completion scripts are extremely hacky. just uses clap to parse arguments, and clap generates the basic completion scripts. We then modify them to make them complete recipe names with the output of just --summary, which includes ::-separated recipe paths in modules.

I would prefer to use space-separated recipe paths, and I would prefer to complete them one at a time, but this just isn't possible with the current approach, since the completion scripts have no idea which arguments are present on the command line so far, so they can't incrementally complete recipes and modules as the user descends into modules.

Fixing this is blocked on clap-rs/clap#1232, which is a proposed improvement to claps's completion engine. I suppose someone could try modifying the completion scripts, but that gets messy fast.

@valscion
Copy link

I would prefer to use space-separated recipe paths, and I would prefer to complete them one at a time, but this just isn't possible with the current approach, since the completion scripts have no idea which arguments are present on the command line so far, so they can't incrementally complete recipes and modules as the user descends into modules.

Fixing this is blocked on clap-rs/clap#1232, which is a proposed improvement to claps's completion engine. I suppose someone could try modifying the completion scripts, but that gets messy fast.

Oh wow what a rabbit hole — I understand now that this isn't such an easy feat 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants