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

Shell completion improvements #6128

Merged
merged 7 commits into from
Apr 19, 2022
Merged

Conversation

ncfavier
Copy link
Member

See individual commits for details.

Regarding d69a111, a possible future improvement would be to rely on the shell for completing paths instead of doing it ourselves. This is non-trivial because we need to be able to complete only directories, and to complete flake names in addition to paths, and I don't know how well bash, zsh and fish support that.

Copy link
Contributor

@tomberek tomberek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builds and functions as expected.

@edolstra
Copy link
Member

I feel tilde expansion should be done in the shell because it's shell-specific, and the shell knows how to do it correctly (e.g. expand ~user).

@ncfavier
Copy link
Member Author

ncfavier commented Feb 21, 2022

It seems like the closest we can get to performing tilde expansion on a word in bash is to use eval, which feels ugly to me... I don't know about the other shells.

EDIT: ugly because I don't expect nix ... $(foo)<Tab> to run foo, especially if it's a long-running command. Maybe we could limit the use of eval to words starting with ~, but still...

@ncfavier
Copy link
Member Author

(I mean in the case of flake fragments. For paths we could just defer to the shell's path completion as said in the OP.)

@tomberek
Copy link
Contributor

Even for flake fragments I believe it can be deferred until the program is run unless it has a “path:” prefix. How about for now allow the shell to handle this for bare paths without a schema?

@ncfavier
Copy link
Member Author

I'm not sure what you mean by "allow the shell to handle this".

Otherwise trying to complete `nix build foo#bar --update-input <Tab>`
fails with "unexpected fragment"
Without this, completing `nix eval -f file.nix foo.<Tab>` suggests `bar`
instead of `foo.bar`, which messes up the command
I was surprised to see an error mentioning ___COMPLETE___ when trying to
complete a flag argument that had no completer implemented
Allows completing `nix why-depends /run/cur<Tab>` to /run/current-system
Allows completing `nix build ~/flake#<Tab>`.
We can implement expansion for `~user` later if needed.
Not using wordexp(3) since that expands way too much.
Requires moving the MixEvalArgs class from libexpr to libcmd because
that's where completeFlakeRef is.
@ncfavier
Copy link
Member Author

ncfavier commented Mar 7, 2022

How can we move this forward? I think this is already an improvement in every situation, do you still want me to look into the eval thing?

Copy link
Member

@Artturin Artturin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pr makes this work

$ nix eval -f repeat.nix bar.<tab>
{ pkgs ? import <nixpkgs> {}}:
{
bar = pkgs.stdenv.mkDerivation {
  name = "hi";
  passthru.okay = "hi";
};
}

Comment on lines +102 to +108
.completer = {[&](size_t n, std::string_view prefix) {
if (n == 0) {
if (auto flakeRef = getFlakeRefForCompletion())
completeFlakeInputPath(getEvalState(), *flakeRef, prefix);
} else if (n == 1) {
completeFlakeRef(getEvalState()->store, prefix);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noting that this only works if --override-input is given after the installable. making it work before the installable may require bigger changes so out of scope for this pr as its already an improvement

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #6693, but note that this is strongly limited. For example, nix build --override-input foo<Tab> my-flake will treat my-flake as the second argument to --override-input and there's not much we can do about that.

@Artturin
Copy link
Member

ping

@Artturin
Copy link
Member

fwiw im using this pr but rebased on to master https://github.com/Artturin/nix/commits/completion-rebased

(self: super: {
  nixVersions = super.nixVersions // {
    unstable = super.nixVersions.unstable.overrideAttrs (oldAttrs: {
      src = super.fetchFromGitHub {
        owner = "artturin";
        repo = "nix";
        rev = "fdda412f2fea22c89163029dc33d3a5f378315fc";
        sha256 = "sha256-oy2JhK+66Tt3ULR2ZJf/ebrLnqvoWLLjHHlr9rpS4Xo=";
      };
    });
  };
})

@edolstra edolstra merged commit 51712bf into NixOS:master Apr 19, 2022
@ncfavier ncfavier deleted the fix-completion branch April 19, 2022 11:45
@tomberek
Copy link
Contributor

Is there something simple that would allow this?

nix eval file:$PWD/thing.tgz#packages.<tab><tab>

@ncfavier
Copy link
Member Author

~ $ nix flake metadata file:/home/n/git/nixpkgs
error: input 'file:/home/n/git/nixpkgs' is unsupported

Doesn't look like a completion problem, but I think you can just remove the file: in that case.

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.

None yet

4 participants