Improve shell completion of flake inputs #6693

ncfavier · 2022-06-20T02:47:23Z

Makes nix build ~/flake1 ~/flake2 --update/override-input <Tab> do the right thing:

do tilde expansion
defer the completion until all arguments are parsed, otherwise _installables is empty and nix assumes we mean the current directory
complete inputs of all given flakes instead of just the first one

For 2, I also considered making arguments like installables "incremental", i.e. adding arguments to _installables as they are parsed, but this turned out to be a mess to implement. Also with the chosen approach we can complete things like nix build --update-input foo<Tab> flake, although this is unreliable (e.g. it won't work with an empty prefix because bash apparently does not insert an empty word there. EDIT: probably a bash bug).

Allow `nix build flake1 flake2 --update-input <Tab>` to complete the inputs of both flakes. Also do tilde expansion so that `nix build ~/flake --update-input <Tab>` works.

ncfavier · 2022-07-08T10:23:51Z

@thufschmitt sorry to ping but it would be nice to have this in the 2.10 milestone as it's just quality of life improvements without changes to any exposed interfaces.

ncfavier · 2022-07-11T09:05:16Z

@thufschmitt sorry to ping but it would be nice to have this in the 2.10 milestone as it's just quality of life improvements without changes to any exposed interfaces.

or @edolstra

thufschmitt

Thanks (and thanks for the ping, I kinda missed the PR the first time).
A couple of small comments on the code, but looks good overall.

My main issue is that this doesn't come with any test. But since there wasn't any before either I don't think that should be a blocker.

thufschmitt · 2022-07-11T12:35:07Z

src/libcmd/command.hh

    MixFlakeOptions();

-    virtual std::optional<FlakeRef> getFlakeRefForCompletion()
+    virtual std::vector<std::string> getFlakesForCompletion()


Any reason why this now return std::strings rather than FlakeRefs? Unless there's a good reason for it, it would be nicer to parse the refs eagerly just to avoid propagating opaque strings in the codebase

The reason is to do tilde expansion in a single place, rather than ask each implementation of getFlakesForCompletion to do it.

I see… I'm not entirely convinced that's the best long-term solution, but that'll definitively do it for the time being

We only complete one argument, right? So I think doing it in getFlakesForCompletion is fine?

I changed this in #8131

thufschmitt · 2022-07-11T12:43:24Z

src/libutil/args.hh

@@ -148,6 +148,8 @@ protected:
       argument (if any) have been processed. */
    virtual void initialFlagsProcessed() {}

+    virtual void doCompletions() { }


The name is a bit misleading since most of the times doCompletions will be empty and the completion will happen elsewhere. Not sure how to call it, but maybe something like completionHook would already be better.

Also, it definitely needs a bit of doc to explicit that it's only used in some cases (not sure how to phrase that properly either tbh 🙃 )

Renamed and added a comment.

I'll try to add tests.

Defers completion of flake inputs until the whole command line is parsed so that we know what flakes we need to complete the inputs of. Previously, `nix build flake --update-input <Tab>` always behaved like `nix build . --update-input <Tab>`.

thufschmitt · 2022-07-11T13:19:27Z

My main issue is that this doesn't come with any test. But since there wasn't any before either I don't think that should be a blocker.

Working on that in another branch

thufschmitt

Thanks, looks good :)

(For future reference: There's a follow-up PR adding tests at #6784)

thufschmitt · 2022-07-12T07:10:31Z

src/libcmd/command.hh

    MixFlakeOptions();

-    virtual std::optional<FlakeRef> getFlakeRefForCompletion()
+    virtual std::vector<std::string> getFlakesForCompletion()


I see… I'm not entirely convinced that's the best long-term solution, but that'll definitively do it for the time being

Revert "Merge pull request NixOS#6693 from ncfavier/complete-flake-inputs" This reverts commit f071eb3, reversing changes made to 7e23039.

@thufschmitt

As I complained in NixOS#6784 (comment) (a comment on the wrong PR, sorry again!), NixOS#6693 introduced a second completions mechanism to fix a bug. Having two completion mechanisms isn't so nice. As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef` to `std::string` when collecting flake refs. Now it is `FlakeRefs` again. The underlying issue that sought to work around was that completion of arguments not at the end can still benefit from the information from latter arguments. To fix this better, we rip out that change and simply defer all completion processing until after all the (regular, already-complete) arguments have been passed. In addition, I noticed the original completion logic used some global variables. I do not like global variables, because even if they save lines of code, they also obfuscate the architecture of the code. I got rid of them moved them to a new `RootArgs` class, which now has `parseCmdline` instead of `Args`. The idea is that we have many argument parsers from subcommands and what-not, but only one root args that owns the other per actual parsing invocation. The state that was global is now part of the root args instead. This did, admittedly, add a bunch of new code. And I do feel bad about that. So I went and added a lot of API docs to try to at least make the current state of things clear to the next person. -- This is needed for RFC 134 (tracking issue NixOS#7868). It was very hard to modularize `Installable` parsing when there were two completion arguments. I wouldn't go as far as to say it is *easy* now, but at least it is less hard (and the completions test finally passed).

@thufschmitt

As I complained in NixOS#6784 (comment) (a comment on the wrong PR, sorry again!), NixOS#6693 introduced a second completions mechanism to fix a bug. Having two completion mechanisms isn't so nice. As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef` to `std::string` when collecting flake refs. Now it is `FlakeRefs` again. The underlying issue that sought to work around was that completion of arguments not at the end can still benefit from the information from latter arguments. To fix this better, we rip out that change and simply defer all completion processing until after all the (regular, already-complete) arguments have been passed. In addition, I noticed the original completion logic used some global variables. I do not like global variables, because even if they save lines of code, they also obfuscate the architecture of the code. I got rid of them moved them to a new `RootArgs` class, which now has `parseCmdline` instead of `Args`. The idea is that we have many argument parsers from subcommands and what-not, but only one root args that owns the other per actual parsing invocation. The state that was global is now part of the root args instead. This did, admittedly, add a bunch of new code. And I do feel bad about that. So I went and added a lot of API docs to try to at least make the current state of things clear to the next person. -- This is needed for RFC 134 (tracking issue NixOS#7868). It was very hard to modularize `Installable` parsing when there were two completion arguments. I wouldn't go as far as to say it is *easy* now, but at least it is less hard (and the completions test finally passed).

@thufschmitt

As I complained in NixOS#6784 (comment) (a comment on the wrong PR, sorry again!), NixOS#6693 introduced a second completions mechanism to fix a bug. Having two completion mechanisms isn't so nice. As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef` to `std::string` when collecting flake refs. Now it is `FlakeRefs` again. The underlying issue that sought to work around was that completion of arguments not at the end can still benefit from the information from latter arguments. To fix this better, we rip out that change and simply defer all completion processing until after all the (regular, already-complete) arguments have been passed. In addition, I noticed the original completion logic used some global variables. I do not like global variables, because even if they save lines of code, they also obfuscate the architecture of the code. I got rid of them moved them to a new `RootArgs` class, which now has `parseCmdline` instead of `Args`. The idea is that we have many argument parsers from subcommands and what-not, but only one root args that owns the other per actual parsing invocation. The state that was global is now part of the root args instead. This did, admittedly, add a bunch of new code. And I do feel bad about that. So I went and added a lot of API docs to try to at least make the current state of things clear to the next person. -- This is needed for RFC 134 (tracking issue NixOS#7868). It was very hard to modularize `Installable` parsing when there were two completion arguments. I wouldn't go as far as to say it is *easy* now, but at least it is less hard (and the completions test finally passed).

@thufschmitt

As I complained in NixOS#6784 (comment) (a comment on the wrong PR, sorry again!), NixOS#6693 introduced a second completions mechanism to fix a bug. Having two completion mechanisms isn't so nice. As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef` to `std::string` when collecting flake refs. Now it is `FlakeRefs` again. The underlying issue that sought to work around was that completion of arguments not at the end can still benefit from the information from latter arguments. To fix this better, we rip out that change and simply defer all completion processing until after all the (regular, already-complete) arguments have been passed. In addition, I noticed the original completion logic used some global variables. I do not like global variables, because even if they save lines of code, they also obfuscate the architecture of the code. I got rid of them moved them to a new `RootArgs` class, which now has `parseCmdline` instead of `Args`. The idea is that we have many argument parsers from subcommands and what-not, but only one root args that owns the other per actual parsing invocation. The state that was global is now part of the root args instead. This did, admittedly, add a bunch of new code. And I do feel bad about that. So I went and added a lot of API docs to try to at least make the current state of things clear to the next person. -- This is needed for RFC 134 (tracking issue NixOS#7868). It was very hard to modularize `Installable` parsing when there were two completion arguments. I wouldn't go as far as to say it is *easy* now, but at least it is less hard (and the completions test finally passed).

@thufschmitt

As I complained in NixOS#6784 (comment) (a comment on the wrong PR, sorry again!), NixOS#6693 introduced a second completions mechanism to fix a bug. Having two completion mechanisms isn't so nice. As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef` to `std::string` when collecting flake refs. Now it is `FlakeRefs` again. The underlying issue that sought to work around was that completion of arguments not at the end can still benefit from the information from latter arguments. To fix this better, we rip out that change and simply defer all completion processing until after all the (regular, already-complete) arguments have been passed. In addition, I noticed the original completion logic used some global variables. I do not like global variables, because even if they save lines of code, they also obfuscate the architecture of the code. I got rid of them moved them to a new `RootArgs` class, which now has `parseCmdline` instead of `Args`. The idea is that we have many argument parsers from subcommands and what-not, but only one root args that owns the other per actual parsing invocation. The state that was global is now part of the root args instead. This did, admittedly, add a bunch of new code. And I do feel bad about that. So I went and added a lot of API docs to try to at least make the current state of things clear to the next person. -- This is needed for RFC 134 (tracking issue NixOS#7868). It was very hard to modularize `Installable` parsing when there were two completion arguments. I wouldn't go as far as to say it is *easy* now, but at least it is less hard (and the completions test finally passed).

@thufschmitt

As I complained in NixOS#6784 (comment) (a comment on the wrong PR, sorry again!), NixOS#6693 introduced a second completions mechanism to fix a bug. Having two completion mechanisms isn't so nice. As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef` to `std::string` when collecting flake refs. Now it is `FlakeRefs` again. The underlying issue that sought to work around was that completion of arguments not at the end can still benefit from the information from latter arguments. To fix this better, we rip out that change and simply defer all completion processing until after all the (regular, already-complete) arguments have been passed. In addition, I noticed the original completion logic used some global variables. I do not like global variables, because even if they save lines of code, they also obfuscate the architecture of the code. I got rid of them moved them to a new `RootArgs` class, which now has `parseCmdline` instead of `Args`. The idea is that we have many argument parsers from subcommands and what-not, but only one root args that owns the other per actual parsing invocation. The state that was global is now part of the root args instead. This did, admittedly, add a bunch of new code. And I do feel bad about that. So I went and added a lot of API docs to try to at least make the current state of things clear to the next person. -- This is needed for RFC 134 (tracking issue NixOS#7868). It was very hard to modularize `Installable` parsing when there were two completion arguments. I wouldn't go as far as to say it is *easy* now, but at least it is less hard (and the completions test finally passed).

@thufschmitt

As I complained in NixOS#6784 (comment) (a comment on the wrong PR, sorry again!), NixOS#6693 introduced a second completions mechanism to fix a bug. Having two completion mechanisms isn't so nice. As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef` to `std::string` when collecting flake refs. Now it is `FlakeRefs` again. The underlying issue that sought to work around was that completion of arguments not at the end can still benefit from the information from latter arguments. To fix this better, we rip out that change and simply defer all completion processing until after all the (regular, already-complete) arguments have been passed. In addition, I noticed the original completion logic used some global variables. I do not like global variables, because even if they save lines of code, they also obfuscate the architecture of the code. I got rid of them moved them to a new `RootArgs` class, which now has `parseCmdline` instead of `Args`. The idea is that we have many argument parsers from subcommands and what-not, but only one root args that owns the other per actual parsing invocation. The state that was global is now part of the root args instead. This did, admittedly, add a bunch of new code. And I do feel bad about that. So I went and added a lot of API docs to try to at least make the current state of things clear to the next person. -- This is needed for RFC 134 (tracking issue NixOS#7868). It was very hard to modularize `Installable` parsing when there were two completion arguments. I wouldn't go as far as to say it is *easy* now, but at least it is less hard (and the completions test finally passed).

@thufschmitt

As I complained in NixOS#6784 (comment) (a comment on the wrong PR, sorry again!), NixOS#6693 introduced a second completions mechanism to fix a bug. Having two completion mechanisms isn't so nice. As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef` to `std::string` when collecting flake refs. Now it is `FlakeRefs` again. The underlying issue that sought to work around was that completion of arguments not at the end can still benefit from the information from latter arguments. To fix this better, we rip out that change and simply defer all completion processing until after all the (regular, already-complete) arguments have been passed. In addition, I noticed the original completion logic used some global variables. I do not like global variables, because even if they save lines of code, they also obfuscate the architecture of the code. I got rid of them moved them to a new `RootArgs` class, which now has `parseCmdline` instead of `Args`. The idea is that we have many argument parsers from subcommands and what-not, but only one root args that owns the other per actual parsing invocation. The state that was global is now part of the root args instead. This did, admittedly, add a bunch of new code. And I do feel bad about that. So I went and added a lot of API docs to try to at least make the current state of things clear to the next person. -- This is needed for RFC 134 (tracking issue NixOS#7868). It was very hard to modularize `Installable` parsing when there were two completion arguments. I wouldn't go as far as to say it is *easy* now, but at least it is less hard (and the completions test finally passed).

@thufschmitt

As I complained in NixOS#6784 (comment) (a comment on the wrong PR, sorry again!), NixOS#6693 introduced a second completions mechanism to fix a bug. Having two completion mechanisms isn't so nice. As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef` to `std::string` when collecting flake refs. Now it is `FlakeRefs` again. The underlying issue that sought to work around was that completion of arguments not at the end can still benefit from the information from latter arguments. To fix this better, we rip out that change and simply defer all completion processing until after all the (regular, already-complete) arguments have been passed. In addition, I noticed the original completion logic used some global variables. I do not like global variables, because even if they save lines of code, they also obfuscate the architecture of the code. I got rid of them moved them to a new `RootArgs` class, which now has `parseCmdline` instead of `Args`. The idea is that we have many argument parsers from subcommands and what-not, but only one root args that owns the other per actual parsing invocation. The state that was global is now part of the root args instead. This did, admittedly, add a bunch of new code. And I do feel bad about that. So I went and added a lot of API docs to try to at least make the current state of things clear to the next person. -- This is needed for RFC 134 (tracking issue NixOS#7868). It was very hard to modularize `Installable` parsing when there were two completion arguments. I wouldn't go as far as to say it is *easy* now, but at least it is less hard (and the completions test finally passed). Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

@thufschmitt

As I complained in NixOS#6784 (comment) (a comment on the wrong PR, sorry again!), NixOS#6693 introduced a second completions mechanism to fix a bug. Having two completion mechanisms isn't so nice. As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef` to `std::string` when collecting flake refs. Now it is `FlakeRefs` again. The underlying issue that sought to work around was that completion of arguments not at the end can still benefit from the information from latter arguments. To fix this better, we rip out that change and simply defer all completion processing until after all the (regular, already-complete) arguments have been passed. In addition, I noticed the original completion logic used some global variables. I do not like global variables, because even if they save lines of code, they also obfuscate the architecture of the code. I got rid of them moved them to a new `RootArgs` class, which now has `parseCmdline` instead of `Args`. The idea is that we have many argument parsers from subcommands and what-not, but only one root args that owns the other per actual parsing invocation. The state that was global is now part of the root args instead. This did, admittedly, add a bunch of new code. And I do feel bad about that. So I went and added a lot of API docs to try to at least make the current state of things clear to the next person. -- This is needed for RFC 134 (tracking issue NixOS#7868). It was very hard to modularize `Installable` parsing when there were two completion arguments. I wouldn't go as far as to say it is *easy* now, but at least it is less hard (and the completions test finally passed). Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

@thufschmitt

As I complained in NixOS#6784 (comment) (a comment on the wrong PR, sorry again!), NixOS#6693 introduced a second completions mechanism to fix a bug. Having two completion mechanisms isn't so nice. As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef` to `std::string` when collecting flake refs. Now it is `FlakeRefs` again. The underlying issue that sought to work around was that completion of arguments not at the end can still benefit from the information from latter arguments. To fix this better, we rip out that change and simply defer all completion processing until after all the (regular, already-complete) arguments have been passed. In addition, I noticed the original completion logic used some global variables. I do not like global variables, because even if they save lines of code, they also obfuscate the architecture of the code. I got rid of them moved them to a new `RootArgs` class, which now has `parseCmdline` instead of `Args`. The idea is that we have many argument parsers from subcommands and what-not, but only one root args that owns the other per actual parsing invocation. The state that was global is now part of the root args instead. This did, admittedly, add a bunch of new code. And I do feel bad about that. So I went and added a lot of API docs to try to at least make the current state of things clear to the next person. -- This is needed for RFC 134 (tracking issue NixOS#7868). It was very hard to modularize `Installable` parsing when there were two completion arguments. I wouldn't go as far as to say it is *easy* now, but at least it is less hard (and the completions test finally passed). Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

@thufschmitt

As I complained in #6784 (comment) (a comment on the wrong PR, sorry again!), #6693 introduced a second completions mechanism to fix a bug. Having two completion mechanisms isn't so nice. As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef` to `std::string` when collecting flake refs. Now it is `FlakeRefs` again. The underlying issue that sought to work around was that completion of arguments not at the end can still benefit from the information from latter arguments. To fix this better, we rip out that change and simply defer all completion processing until after all the (regular, already-complete) arguments have been passed. In addition, I noticed the original completion logic used some global variables. I do not like global variables, because even if they save lines of code, they also obfuscate the architecture of the code. I got rid of them moved them to a new `RootArgs` class, which now has `parseCmdline` instead of `Args`. The idea is that we have many argument parsers from subcommands and what-not, but only one root args that owns the other per actual parsing invocation. The state that was global is now part of the root args instead. This did, admittedly, add a bunch of new code. And I do feel bad about that. So I went and added a lot of API docs to try to at least make the current state of things clear to the next person. -- This is needed for RFC 134 (tracking issue #7868). It was very hard to modularize `Installable` parsing when there were two completion arguments. I wouldn't go as far as to say it is *easy* now, but at least it is less hard (and the completions test finally passed). Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

Complete flake inputs for all given flakes

d6d0e78

Allow `nix build flake1 flake2 --update-input <Tab>` to complete the inputs of both flakes. Also do tilde expansion so that `nix build ~/flake --update-input <Tab>` works.

ncfavier mentioned this pull request Jun 20, 2022

Shell completion improvements #6128

Merged

thufschmitt requested changes Jul 11, 2022

View reviewed changes

ncfavier force-pushed the complete-flake-inputs branch from df6fe6b to 589e7bd Compare July 11, 2022 13:12

ncfavier force-pushed the complete-flake-inputs branch from 589e7bd to 711b2e1 Compare July 11, 2022 13:13

thufschmitt mentioned this pull request Jul 11, 2022

Add some tests for the CLI completion #6784

Merged

thufschmitt approved these changes Jul 12, 2022

View reviewed changes

thufschmitt merged commit f071eb3 into NixOS:master Jul 12, 2022

ncfavier deleted the complete-flake-inputs branch July 12, 2022 08:36

Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 29, 2023

Overhaul completions, redo NixOS#6693

8bb147d

Revert "Merge pull request NixOS#6693 from ncfavier/complete-flake-inputs" This reverts commit f071eb3, reversing changes made to 7e23039.

Ericson2314 mentioned this pull request Mar 29, 2023

Overhaul completions, redo #6693 #8131

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve shell completion of flake inputs #6693

Improve shell completion of flake inputs #6693

ncfavier commented Jun 20, 2022 •

edited

ncfavier commented Jul 8, 2022

ncfavier commented Jul 11, 2022

thufschmitt left a comment

thufschmitt Jul 11, 2022

ncfavier Jul 11, 2022

thufschmitt Jul 12, 2022

Ericson2314 Mar 29, 2023

Ericson2314 Mar 29, 2023

thufschmitt Jul 11, 2022

ncfavier Jul 11, 2022

thufschmitt commented Jul 11, 2022

thufschmitt left a comment

thufschmitt Jul 12, 2022

Improve shell completion of flake inputs #6693

Improve shell completion of flake inputs #6693

Conversation

ncfavier commented Jun 20, 2022 • edited

ncfavier commented Jul 8, 2022

ncfavier commented Jul 11, 2022

thufschmitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thufschmitt commented Jul 11, 2022

thufschmitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncfavier commented Jun 20, 2022 •

edited