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

Enable Zsh completions #34

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Conversation

j-g00da
Copy link

@j-g00da j-g00da commented Feb 4, 2025

No description provided.

@j-g00da j-g00da marked this pull request as draft February 4, 2025 20:25
@j-g00da j-g00da marked this pull request as ready for review February 4, 2025 20:45
@j-g00da j-g00da changed the title Zsh completions Enable Zsh completions Feb 4, 2025
Copy link
Contributor

@bswck bswck left a comment

Choose a reason for hiding this comment

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

Thanks!

src/duty/cli.py Outdated Show resolved Hide resolved
src/duty/completions.zsh Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
Don't require changes when adding support for completion in other shells.

Co-authored-by: Bartosz Sławecki <[email protected]>
Copy link
Owner

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

Thanks a lot @j-g00da! It's really cool to get completions for Zsh too 😄

One thing I'd like to see for Zsh completions is actual description of the completion words: Zsh's completion system allows to attach a description to each term/word, that is then displayed when hitting TAB in the shell. That means we'd have to add a bit of code to our logic for generating word candidates here, so that descriptions are returned too. I see two options:

  • the logic always returns tuples (word, description), and higher-up in the stack we filter out descriptions if the shell doesn't support those
  • the logic accepts a shell argument that lets it know whether it should return descriptions as well as words

We would have to check how this integrates with the actual completion script (completions.zsh) and compctl command.

docs/usage.md Outdated
Or in Zsh with:

```zsh
completions_dir="$HOME/.duty"
Copy link
Owner

Choose a reason for hiding this comment

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

I understand you may have taken inspiration from the Bash example above, but this doesn't seem right to me. The Bash example uses standard locations that Bash immediately understands. This location would require users to fiddle with the Zsh configuration to load the completion from ~/.duty I believe. Furthermore, not using standard locations is generally frowned-upon by users (me included 😄) as that leads to a cluttered HOME directory 🙂

In short, could you try to see if there's a standard location we could write the file in, so that Zsh's completion system natively finds it?

Copy link
Author

Choose a reason for hiding this comment

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

In short, could you try to see if there's a standard location we could write the file in, so that Zsh's completion system natively finds it?

From what I understand, zsh doesn't have any "standard" location for completions other than /usr/local/share/zsh/site-functions, which is in fpath by default, but I don't think we want to install it system-wide. I based my approach on oh my zsh, which puts completions under ~/.oh-my-zsh/completions and adds it to fpath. Still, I think cluttered HOME is a good point. Let me know what you think.

One thing I'd like to see for Zsh completions is actual description of the completion words

Good point, I will work on this later today.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks! We could then document that and let the user decide. I found https://github.com/zsh-users/zsh-completions/blob/master/zsh-completions-howto.org#telling-zsh-which-function-to-use-for-completing-a-command to be very readable, maybe we could link to it.

Copy link
Author

@j-g00da j-g00da Feb 6, 2025

Choose a reason for hiding this comment

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

I've modified the docs yestarday and changed the completions.zsh to use compdef (rich completions) instead of old compctl. Still no descriptions, but now it will be possible to add them. Will work on adding descriptions later today or tomorrow. You can take a look at the docs and let me know if it's enough information (I will polish the text later).
Sources: this answer on stackoverflow, and ofc the zsh-completions-howto.org.

Copy link
Author

@j-g00da j-g00da Feb 6, 2025

Choose a reason for hiding this comment

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

Also - completions.zsh is WIP, it's just a minimal example for now, but it works, and I know opts.complete parsing as of now breaks the bash complete implementation.
Also 2 - We can make it compatible with old zsh versions, but I don't know if this is that important since user can always use bash completions in zsh if native approach doesn't work (explained in the docs).

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you so much! Don't bother about supporting old Zsh versions, latest is fine 🙂

@j-g00da j-g00da marked this pull request as draft February 5, 2025 13:19
@bswck
Copy link
Contributor

bswck commented Feb 6, 2025

I'm yearning for some automatic tests of those scripts. WDYT @pawamoy

@pawamoy
Copy link
Owner

pawamoy commented Feb 6, 2025

@bswck yes, testing is always good! I'm definitely not sure how to test auto-completion though! Any idea? Wouldn't it be enough to just test the output of our Python code that generates words?

@bswck
Copy link
Contributor

bswck commented Feb 7, 2025

Wouldn't it be enough to just test the output of our Python code that generates words?

Definitely no, I strongly feel we need white-box regression tests of completions working on the user end.

I'm definitely not sure how to test auto-completion though! Any idea?

We can get some inspiration from typer tests which seem to do exactly that.

@j-g00da
Copy link
Author

j-g00da commented Feb 9, 2025

I think of doing a bit of extra work on it, since the docs on how to enable completions grew in the Zsh section.

Duty is simple, and installing completions for it should also be simple. It’s fine to inform the user about options, but for the most part… it's too much and I really would like something like duty --install-completions that would just install it in some default place, for auto-detected shell so I don't have to think about it.

For Bash, it would be really easy - just run what’s in the docs. For Zsh I think the best option would be to put it in /usr/local/share/zsh/site-functions, because:

  1. This is what most tools use - homebrew, uv, cargo to name a few.
  2. Doesn't require adding to fpath, as it is there by default; which normally would be tricky, as compinit must be run after adding a dir to fpath, so we would have to parse .zshrc and do some unholy stuff or ask user to manually do it, which misses the point.

In fact, I see only one disadvantage of such an approach - /usr/local/share is by default owned by root on linux. This is not a problem on macos, where /usr/local/share is owned by the current user. So, in short - this command would require sudo on Linux. I still think it's 100% worth it, let me know what you think @pawamoy, @bswck.

@j-g00da
Copy link
Author

j-g00da commented Feb 9, 2025

Also - I just checked Typer source code, since @bswck linked it, and this is exactly what they do:

  • --install-completion: Install completion for the current shell.
  • --show-completion: Show completion for the current shell, to copy it or customize the installation.

But when it comes to installation dir, it's ~/.zfunc. So as I said, there really is no "default" directory for that with Zsh...
source:

def install_zsh(*, prog_name: str, complete_var: str, shell: str) -> Path:
    # Setup Zsh and load ~/.zfunc
    zshrc_path = Path.home() / ".zshrc"
    zshrc_path.parent.mkdir(parents=True, exist_ok=True)
    zshrc_content = ""
    if zshrc_path.is_file():
        zshrc_content = zshrc_path.read_text()
    completion_line = "fpath+=~/.zfunc; autoload -Uz compinit; compinit"
    if completion_line not in zshrc_content:
        zshrc_content += f"\n{completion_line}\n"
    style_line = "zstyle ':completion:*' menu select"
    # TODO: consider setting the style only for the current program
    # style_line = f"zstyle ':completion:*:*:{prog_name}:*' menu select"
    # Install zstyle completion config only if the user doesn't have a customization
    if "zstyle" not in zshrc_content:
        zshrc_content += f"\n{style_line}\n"
    zshrc_content = f"{zshrc_content.strip()}\n"
    zshrc_path.write_text(zshrc_content)
    # Install completion under ~/.zfunc/
    path_obj = Path.home() / f".zfunc/_{prog_name}"
    path_obj.parent.mkdir(parents=True, exist_ok=True)
    script_content = get_completion_script(
        prog_name=prog_name, complete_var=complete_var, shell=shell
    )
    path_obj.write_text(script_content)
    return path_obj

On a side note they might have a potential bug there...

@pawamoy
Copy link
Owner

pawamoy commented Feb 9, 2025

Completely agree with your comment @j-g00da. I was exactly going to say that the only downside is that users will probably have to use sudo. IMO that's an acceptable tradeoff. If they don't want to use sudo, they can deal with their own configuration by writing the completion script where they prefer.

OK so it looks like Typer modifies .zshrc (in addition to cluttering home with a non-standard .zfunc directory it seems) 🤢 Please let us not do that. EDIT: PDM also suggests writing in ~/.zfunc, but I can't find any official docs about such a directory, so it looks like a convention?

Lets go with the approach you suggested:

  • an --install-completions flag detects the shell (and accepts the shell as argument for user control)
  • for Bash it runs the example we currently have (using equivalent instructions in Python)
  • for Zsh it tries to write in /usr/local/share/zsh/site-functions, and the docs say that one must probably run the command with sudo

@pawamoy
Copy link
Owner

pawamoy commented Feb 9, 2025

@bswck @j-g00da do you know if Python has any standard or discussion about distributing shell completions inside packages (source dists or wheels)? Looks like venvs have the opportunity to store a, I don't know, completions folder somewhere. Then tool installers like pipx and uv could expose command to easily locate / load such completions. Each package could distribute completions for all (supported) shells, so that users actually never have to run commands to either install completions or redirect completions. CLIs could then free themselves of --completion-related flags.

I mean pipx managed to handle manpages so surely they could handle completion scripts 🤔

Basher offers such a mechanism for example. Your project defines completion files for Bash, Zsh, whatever, and Basher puts them in a dedicated folder. Users can then easily point their shell at these folders.

@pawamoy
Copy link
Owner

pawamoy commented Feb 9, 2025

I opened pypa/pipx#1604 and astral-sh/uv#11354, let see what people think 😊

@bswck
Copy link
Contributor

bswck commented Feb 9, 2025

I opened pypa/pipx#1604 and astral-sh/uv#11354, let see what people think 😊

Related pawamoy/git-changelog#66 (comment)

@j-g00da
Copy link
Author

j-g00da commented Feb 10, 2025

EDIT: PDM also suggests writing in ~/.zfunc, but I can't find any official docs about such a directory, so it looks like a convention?

Poetry suggests .zfunc too, but also lists omz directory as a good option.

Still - it's not backed by any standard AFAIK (it's often described as example of custom directory for completions) and because of that, the directory is not by default in fpath and requires adding it manually in .zshrc. Maybe I'm too deep into this, but I still have strong feelings that site-functions would be the best place for that. On the other hand, since it's popular, we can use .zfunc as an example of custom dir instead of a random .duty. Let me know what you think.

I still want to take a look into uv source code later, on my mac it installed completions in site-functions, I want to know if it does the same for linux.

@j-g00da
Copy link
Author

j-g00da commented Feb 10, 2025

  • the logic always returns tuples (word, description), and higher-up in the stack we filter out descriptions if the shell doesn't support those

I went with this approach in 592f938

image

@pawamoy
Copy link
Owner

pawamoy commented Feb 10, 2025

Awesome 😍

Comment on lines 31 to 35
# We only have space for one line of description,
# so I remove descriptions of sub-command parameters from help_text
# by removing everything after the first newline.
# I don't think it is the best approach and should be discussed.
return f"{completion}: {help_text or '-'}".split("\n", 1)[0]
Copy link
Owner

Choose a reason for hiding this comment

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

  • functions docstrings: good, it's reasonable to keep only the first line, as docstrings' first line should be a complete sentence anyway.
  • parameter docstrings: we don't parse them yet anyway (from looking at code below). When we do (using Griffe?), I don't think there's a more robust way than keeping the first line anyway. We don't want to enter natural language processing territory (splitting sentences is hard). Maybe we could recommend to use single-line descriptions for parameters, or at least to have a complete sentence as first line.

src/duty/collection.py Show resolved Hide resolved
Copy link
Contributor

@bswck bswck left a comment

Choose a reason for hiding this comment

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

Let's group behaviors by shells instead of topics.

Instead of fine-grained repetitive interfaces

topic1

  • bash
  • zsh
  • other shells...

topic2

  • bash
  • zsh
  • other shells...

we want modular strategies, like

bash

  • topic1
  • topic2

zsh

  • topic1
  • topic2

other shells follow the same pattern...

This approach is known under many names; the I from SOLID, the strategy design pattern, the loose coupling, the high cohesion.

Notice that the current solution doesn't really leverage the use of classes for grouping methods by common state; an interface for a "shell" can be easily described with an abstract class/protocol, distinct implementations gathered within a class-level registry modified via __init_subclass__ of the abstract class/protocol. That would cause future implementations of other shells to be cohesive, discoverable, comprehensive and complete, with a potential of sharing useful state across "topic" methods; something along the lines of:

class Shell(metaclass=abc.ABCMeta):
    name: ClassVar[str]
    implementations: Final[ClassVar[dict[str, Shell]]] = {}

    @abc.abstractmethod
    def parse_completions(self, candidates: Sequence[CompletionCandidateType]) -> str:
        ...

    @abc.abstractmethod
    def install_completions(self, candidates: Sequence[CompletionCandidateType]) -> str:
        ...

    def __init_subclass__(cls) -> None:
        cls.implementations[cls.name] = cls


class Bash(Shell):
    name = "bash"

    def parse_completions(self, candidates: Sequence[CompletionCandidateType]) -> str:
        # implementation for bash

    def install_completions(self) -> str:
        # implementation for bash


class Zsh(Shell):
    name = "zsh"

    def parse_completions(self, candidates: Sequence[CompletionCandidateType]) -> str:
        # implementation for zsh

    def install_completions(self) -> str:
        # implementation for zsh


# other shells follow the same pattern

Implementing a new shell/changing an existing shell implementation will only require changes in one area in the code, instead of a number of them, all over the place, separated by other, unrelated fragments.
Notice how this approach makes the new implementations also portable and easier to plug into the pre-existing runtime—simply implement a strategy for your shell and ship it. The library can find it in the Shell.implementations registry and you can override the existing ones as well, and then maybe suggest a patch to upstream.

I'm up for pair programming on this one, hit me up on Discord if you're interested :)

src/duty/completion.py Outdated Show resolved Hide resolved
src/duty/completion.py Outdated Show resolved Hide resolved
src/duty/completion.py Outdated Show resolved Hide resolved
src/duty/completion.py Outdated Show resolved Hide resolved
Co-authored-by: Bartosz Sławecki <[email protected]>
@j-g00da j-g00da requested a review from bswck February 13, 2025 15:34
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