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: carapace completion for all major shells #1436

Conversation

HirschBerge
Copy link
Contributor

@HirschBerge HirschBerge commented Oct 9, 2024

Type of change

  • Bug fix
  • Feature
  • Documentation update

Description

I saw that there was bash and zsh completion available (which seems to be non-exhaustive of flags, I noticed that while comparing to --help) and as I am a NuShell user primarily, I wanted to have that too. Carapace is a cross-shell (and even windows) auto-completion engine, so I figured it would be nice for everyone to have.

I am not sure if Carapace-spec is required for this as I used documentation from that version, but I could not test as I am on NixOS and the only Carapace available just works. If others could test and confirm, I can make note of that in the READMe. General testing would be handy of course. :)

Checklist

  • any anime playing
  • bumped version

I didn't change the actual code so this all should be identical

  • next, prev and replay work
  • -c history and continue work
  • -d downloads work
  • -s syncplay works
  • -q quality works
  • -v vlc works
  • -e select episode works
  • -S select index works
  • -r range selection works
  • --skip ani-skip works
  • --skip-title ani-skip title argument works
  • --no-detach no detach works
  • --dub and regular (sub) mode both work
  • all providers return links (not necessarily on a single anime, use debug mode to confirm)

  • -h help info is up to date
  • Readme is up to date
  • Man page is up to date

Additional Testcases

  • The safe bet: One Piece
  • Episode 0: Saenai Heroine no Sodatekata ♭
  • Unicode: Saenai Heroine no Sodatekata ♭
  • Non-whole episodes: Tensei shitara slime datta ken (ep. 24.5, ep. 24.9)

@HirschBerge HirschBerge changed the title Add carapace, multi shell autocomplete feat:Add carapace, multi shell autocomplete Oct 9, 2024
@port19x
Copy link
Collaborator

port19x commented Oct 9, 2024

Why would we generate completion when we already have it? Nope

@port19x port19x closed this Oct 9, 2024
@HirschBerge
Copy link
Contributor Author

HirschBerge commented Oct 9, 2024

Why would we generate completion when we already have it? Nope

What are you talking about? You are not doing that... There are more shells than just Bash and Zsh. If you read my description I specifically give the use case.

Please reopen and give this a read-through before insta-closing. The title may be self-explanatory, but I would think reading a PR should be a given

@HirschBerge
Copy link
Contributor Author

Why would we generate completion when we already have it? Nope

TL;DR being and less a-holey than my response above, it is optional, so having more choices for what people use is always good, it's the OSS way, after all. Though, more importantly, it covers any shell you may use, not the two that you use.

@port19x
Copy link
Collaborator

port19x commented Oct 9, 2024

I'll leave it up to the other maintainers to endorse this, but I'm still against inclusion since this adds another dependency.
Altough it is a highly optional one I don't like adding more dependencies, neither to the script nor our CI setup or whatever

@HirschBerge
Copy link
Contributor Author

I'll leave it up to the other maintainers to endorse this, but I'm still against inclusion since this adds another dependency.
Altough it is a highly optional one I don't like adding more dependencies, neither to the script nor our CI setup or whatever

Saying this adds a dependency seems a bit off the mark.
No moreso than the other two which have their own "dependencies." At the end of the day, it's just providing a file to people who already use a tool, it doesn't really even add an optional dep.

Would appreciate a re-open for visibility instead of it laying dorman as closed

@port19x port19x reopened this Oct 9, 2024
@HirschBerge
Copy link
Contributor Author

Thank you, much

@HirschBerge HirschBerge changed the title feat:Add carapace, multi shell autocomplete feat: carapace completion for all major shells Oct 9, 2024
@HirschBerge
Copy link
Contributor Author

Additionally, if it would be more inline with keeping requirements and CI under control, I could translate what I have for carapace into a simple nushell script such as defined here (though very minimal, I could make this actually more useful) to replace or compliment the carapace completion. This would bring down the encompassing nature of working for almost all shells, but still add another major shell's completion.

Let me know if this sounds more reasonable.

@Derisis13
Copy link
Collaborator

Carapace seems to be pretty new, according to repology, not many repos have it (literally none of the stable distros). This eliminates even an optional dependency (for binary packages), as it would be irresolveable. Thus the yaml file would not be packaged at all, so the only people who would use it are the ones who get it from the repo.

I have minimal knowledge (I see traffic on the repo and the copr repo, but don't know how to translate them to installations) on what percentage of our users would even receive the script and how many of them would use it in the end. We can run a poll on our discord to try to get a guess, but I'm not sure if that's a good idea. If we do this we should also poll for the bash and zsh completion scripts.

In the end I'm not even sure if completion should be given by the application. I'd much rather put it in a *shell plugin (for example oh-my-zsh for zsh, I'm pretty sure there's a plugin manager for the relevant ones). Putting code in user directories (during the installation process, except to the program installation directory) is something I would want to avoid.

@HirschBerge
Copy link
Contributor Author

Carapace seems to be pretty new, according to repology, not many repos have it (literally none of the stable distros). This eliminates even an optional dependency (for binary packages), as it would be irresolveable. Thus the yaml file would not be packaged at all, so the only people who would use it are the ones who get it from the repo.

Your link is either dead or timed out while trying to load.
Fair though. I was suggesting that to reduce the need to have multiple different shell-specific completion files as is the current method. Carapace seemed the best since it has a lot of popularity. It's been in the nix stable since I believe either 23.5 or 23.11 and I see a lot of people have it declared.

I have minimal knowledge (I see traffic on the repo and the copr repo, but don't know how to translate them to installations) on what percentage of our users would even receive the script and how many of them would use it in the end. We can run a poll on our discord to try to get a guess, but I'm not sure if that's a good idea. If we do this we should also poll for the bash and zsh completion scripts.

Honestly, as it doesn't add any dependencies and minimal upkeep, I'd say if one person was benefited, it's worth including.

In the end I'm not even sure if completion should be given by the application. I'd much rather put it in a *shell plugin (for example oh-my-zsh for zsh, I'm pretty sure there's a plugin manager for the relevant ones). Putting code in user directories (during the installation process, except to the program installation directory) is something I would want to avoid.

This seems overly complicated unless I misunderstans what you mean. A whole plugin?
I also do not understand what you mean by putting code in user directories?

What about my comment from last week? Would pivoting to this route, which is purely sourcing a file that doesn't even require a shell plugin like the Zsh (and bash idk) method already in the repo does, and possibly linking to this pull for anyone who does already use carapace or in FAQ?

Additionally, if it would be more inline with keeping requirements and CI under control, I could translate what I have for carapace into a simple nushell script such as defined here (though very minimal, I could make this actually more useful) to replace or compliment the carapace completion. This would bring down the encompassing nature of working for almost all shells, but still add another major shell's completion.
Let me know if this sounds more reasonable.

@port19x
Copy link
Collaborator

port19x commented Oct 17, 2024

This qualifies as CI, so it does not require a version bump of the script

@HirschBerge HirschBerge deleted the Add-Carapace,-multi-shell-autocomplete branch October 18, 2024 19:37
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.

3 participants