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

feat: add pixi run completion for fish shell #1680

Merged

Conversation

dennis-wey
Copy link
Contributor

@dennis-wey dennis-wey commented Jul 26, 2024

Add one more line to the fish script for code completion to support listing tasks in pixi run.

I didn't know how to add description information but as far as I saw for the other shells, it's not yet used there either.

Closes #1191

@dennis-wey dennis-wey changed the title add pixi run completion for fish shell feat: add pixi run completion for fish shell Jul 26, 2024
@ruben-arts ruben-arts requested a review from tdejager July 26, 2024 11:53
@dennis-wey dennis-wey force-pushed the feature/add_pixi_run_completion_for_fish branch from b9e37b7 to 95d012b Compare July 26, 2024 12:16
@tdejager
Copy link
Contributor

tdejager commented Jul 26, 2024

Hi!

Thanks for the contribution, that would be a great addition, couple things need to be changed:

  1. As it currently stands, the command currently errors. i.e when evaluating inside of pixi project. This is because it needs to get the arguments from standard err instead of out.
  2. It needs to handle proper error behavior: When pixi task list is run in a folder without a pixi.toml, this command will output an error to stderr and have a exit code of 1. This needs to be handled by ignoring that message. Bash and zsh have examples for it.
  3. It needs to have delay the evaluation, now it will evaluate on the source of the script, so I think it needs quoting around the command.
  4. It needs a completion per line I think, from my testing, rendering it into a string with spaces makes it consider it one command? Not sure about this one.
  5. Last thing, guess its a bit tricky but, it should not autocomplete on pixi run start run <TAB> this should do nothing. This works for bash at least.

Should be close to (but not correct yet):

complete -c pixi -f -n "__fish_seen_subcommand_from run" -a "(pixi task list --machine-readable | string split ' ')"

But that does not handle 2, and 6 though.

For 6. chatgpt suggested something like:

# Function to check if 'run' is the first subcommand
function __fish_pixi_run_first_subcommand
    # Get the current command line input
    set -l cmd (commandline -opc)
    
    # Check if the command line is exactly 'pixi run' or starts with 'pixi run '
    if string match -q "pixi run" -- $cmd
        return 0
    end

    return 1
end

For the -n flag like:

complete -c pixi -f -n "__fish_pixi_run_first_subcommand" -a "(pixi task list --machine-readable | string split ' ')" 

But that does not seem to work alas...

EDIT:

  1. changed in a recent version so crossed that one out, sorry about that.

@dennis-wey
Copy link
Contributor Author

Thanks @tdejager for the pointers!

I did some changes:

  1. For me the newest commit works with error suppression. But can you test if the behavior is as you would expect it?
  2. Added that as well
  3. -a accepts a single string of options devided by space. In my tests the commands get parsed correctly

Regarding Point 5:
I'm not sure what we want to achieve here. Reading from the chat gpt commands:

  • check on command is exactly pixi run
    • no false positives but we would loose completion for pixi run -e dev <TAB>
      • (maybe there is a way to forward the environment to pixi task list, but I don't think we should do this in this pr)
  • check on command starts with pixi run
    • that doesn't help for pixi run start run <TAB>

Instead we could do something like -n "__fish_seen_subcommand_from run; and not __fish_seen_run_multiple_times".
But the we should somehow filter out cases like pixi run -e run <TAB>. But even if we do we might still get false negatives when someone tries to use piping/combining commands.

Another thing is even without this pr we already have this issue:
pixi run python --manifest-<TAB> -> pixi run python --manifest-path

So having thought about that, do you think cases like pixi run start run <TAB> are worth adding this complexity?

Let me know what you think

@tdejager
Copy link
Contributor

@dennis-wey yes this is great thanks! I don't want to block on 5., as its difficult to get right indeed and having the completion is just too nice t.b.h.

Last request, what do you think adding the complete command for the alias r as well, I use that mostly, don't know if that would give too many false positives. wdyt?

@dennis-wey dennis-wey force-pushed the feature/add_pixi_run_completion_for_fish branch 2 times, most recently from 86bc8d6 to f4081d9 Compare July 29, 2024 08:22
@dennis-wey
Copy link
Contributor Author

@tdejager actually I didn't know that that one exists :D
Seems like it was also overlooked in the current auto-completion.

I added a regex pattern that r is treated the same way as run.

@dennis-wey
Copy link
Contributor Author

I'm also not sure why the tests are failing. COuld this be unrelated to my pr?

@dennis-wey dennis-wey force-pushed the feature/add_pixi_run_completion_for_fish branch from f4081d9 to 10baccc Compare July 29, 2024 09:34
@tdejager
Copy link
Contributor

I'm also not sure why the tests are failing. COuld this be unrelated to my pr?

Not its because of the setuptools problems everywhere, hang on!

@tdejager
Copy link
Contributor

Updating with main branch to fix CI.

@tdejager tdejager merged commit 679ea7d into prefix-dev:main Jul 29, 2024
24 checks passed
@dennis-wey dennis-wey deleted the feature/add_pixi_run_completion_for_fish branch July 29, 2024 14:43
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.

pixi run shell completion doesn't work with fish
2 participants