Overhaul completions, redo #6693

[Ericson2314]

Motivation

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.

Context

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).

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

• agreed on idea
• agreed on implementation strategy
• tests, as appropriate
  • functional tests - tests/**.sh
  • unit tests - src/*/tests
  • integration tests - tests/nixos/*
• documentation in the manual
• code and comments are self-explanatory
• commit message explains why the change was made
• new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

[ncfavier]

Regressions:

$ nix flake show ~/flake --update-input <Tab>
(no output)
$ nix run ~/flake --update-input <Tab>
(no output)

Basically we only get tilde expansion for InstallablesCommands now (note the s). I think the whole reason I made getFlakesForCompletion return a string was so that the completion code could take care of tilde expansion instead of doing it in every class.

[Ericson2314]

@ncfavier Thanks. Let's try to get those in the test prior to merging this PR I think.

Basically we only get tilde expansion for InstallablesCommand now (note the s).

Thanks, that should be an easy enough fix.

I think the whole reason I made getFlakesForCompletion return a string was so that the completion code could take care of tilde expansion instead of doing it in every class.

Oh hmm I didn't think it looked that way, but I'll think about this.

[Ericson2314]

@ncfavier Hmm I am confused. I added tests based on what you said, but I didn't see any regressions.

[Ericson2314]

OK, now it is fixed :). Thanks again, @ncfavier, those tests proved quiete helpful in catching both places that needed to change.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-03-31-nix-team-meeting-minutes-45/27002/1

[fricklerhandwerk]

Reviewed with @tomberek.

Overall the change is good, but it needs a bit of clarification for us mortals who don't dream in C++ (yet).

Would be great to have the new docstrings as a separate commit for easier bisection.

[fricklerhandwerk]

I'd need to review it again in full as it seems to have changed quite a bit, and I'm not sure I'll find enough time for it in the next week given other priorities. But my clarification requests were addressed in code comments, so I'm almost at peace with this change. @tomberek could you take it over the finish line?

[Ericson2314]

Sounds good

seems to have changed quite a bit

I don't think I changed it a lot? Mean thing is I landed the tests and API docs for existing stuff first. The rest should be more or less the same (just small things in response to review comments). At least I don't remember doing anything big!

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-10-16-nix-team-meeting-minutes-95/34396/1

[fricklerhandwerk]

Okay, now that I got the quiet to look at it properly, this looks very good. The doc comments help a lot with following the code, and the updated names don't stick out any more.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-10-23-nix-team-meeting-minutes-97/34561/1