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 nushell completion #1599

Merged
merged 5 commits into from
Jul 16, 2024

Conversation

Hofer-Julian
Copy link
Contributor

  • Add nushell completion
  • Stop guessing shell, and make --shell required
  • Adapt instructions

I have a version local that still tries to guess the shell from the $SHELL env var. But it made the code much more verbose, and I am not a fan of that behavior. On my system, $SHELL gives /bin/bash even though I use nushell.

I also adapted the instructions to stop suggesting that your choice of shell is platform dependent. Every listed shell can be used on Windows and vice versa.

- Add nushell completion
- Stop guessing shell, and make `--shell` required
- Adapt instructions

I have a version local that still tries to guess the shell from the `$SHELL` env var.
But it made the code much more verbose, and I am not a fan of that behavior.
On my system `$SHELL` gives `/bin/bash` even though I use nushell.

I also adapted the instructions to stop suggesting that your choice of shell is platform dependent.
Every listed shell can be used on Windows and vice-versa.
@Hofer-Julian Hofer-Julian requested a review from ruben-arts July 10, 2024 07:07
Copy link
Contributor

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

A a rule of thumb I think we should always prefer sane defaults over having to specify a value explicitly because its something else to understand or learn.

I think having to specify the --shell explicitly moves the burden to specify the shell to the user whereas most of the time we can just detect it. If the shell is not properly detected I would consider that a bug instead. Perhaps you can use the logic in rattler_shell to detect the active shell? I think it contains more logic than just checking $SHELL? It also properly detects the shell when using pixi shell. I would be fine with erroring out if the shell cannot be detected though, instead of defaulting to bash.

I also think that although you are technically correct that any shell can be used on Windows we should still keep the tabs in the documentation. I think most users will not be running fish or zsh under windows. And the ones that do will most likely also understand to look under the linux tab. Or we could add a note about it.

Besides that for a starting user having just a single line (especially on Windows) is much less scary than having a whole bunch of echo statements. We want to make it as easy as possible for users.

@Hofer-Julian
Copy link
Contributor Author

A a rule of thumb I think we should always prefer sane defaults over having to specify a value explicitly because its something else to understand or learn.

In general, I agree. For this specific case, I argue we gain nothing.
The actual command to use completions is shell specific anyway.
I don't see how echo 'eval "$(pixi completion)"' >> ~/.bashrc is better than echo 'eval "$(pixi completion --shell bash)"' >> ~/.bashrc. Especially if the former is less robust.

I also think that although you are technically correct that any shell can be used on Windows we should still keep the tabs in the documentation. I think most users will not be running fish or zsh under windows. And the ones that do will most likely also understand to look under the linux tab. Or we could add a note about it.

Thanks for the feedback, I restructured the docs and like it much more now.
Let me know if you agree.

@Hofer-Julian Hofer-Julian requested a review from baszalmstra July 10, 2024 09:18
Copy link
Contributor

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

Ok you convinced me, lets just merge and see if someone complains.

Ill let @ruben-arts review the documentation changes. They LGTM.

Hofer-Julian added a commit to prefix-dev/rattler-build that referenced this pull request Jul 11, 2024
Hofer-Julian added a commit to prefix-dev/rattler-build that referenced this pull request Jul 11, 2024
baszalmstra pushed a commit to prefix-dev/rattler-build that referenced this pull request Jul 11, 2024
@ruben-arts
Copy link
Contributor

image
Any idea why the help is double and prefixing the login and logout?

@Hofer-Julian
Copy link
Contributor Author

image Any idea why the help is double and prefixing the login and logout?

No, but I found similar bugs in rattler-build. Still have to check if it's our or clap_complete_nushell's fault.

Can you reproduce it with other shells?

@tdejager
Copy link
Contributor

image Any idea why the help is double and prefixing the login and logout?

No, but I found similar bugs in rattler-build. Still have to check if it's our or clap_complete_nushell's fault.

Can you reproduce it with other shells?

Well they are valid commands and provide the help for those given commands. E.g. pixi auth help login gives me:

Store authentication information for a given host

Usage: pixi auth login [OPTIONS] <HOST>

Arguments:
  <HOST>  The host to authenticate with (e.g. repo.prefix.dev)

So in that case it seems the docs-string is just wrong, and we might need to ignore these commands in the autcomplete if that possible?

@Hofer-Julian
Copy link
Contributor Author

So in that case it seems the docs-string is just wrong, and we might need to ignore these commands in the autcomplete if that possible?

Nice find @tdejager.
Sounds to me like we can merge this PR then and create a follow-up issue for the issues you found. Agreed?

@ruben-arts ruben-arts merged commit bbe5bab into prefix-dev:main Jul 16, 2024
27 checks passed
@Hofer-Julian Hofer-Julian deleted the nushell-completions branch July 17, 2024 09:31
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.

4 participants