Re: PATCH: got (gameoftrees) suite completion

[Oliver Kiddle <opk@xxxxxxx>]

Mikhail Pchelin wrote:
> I wasn't able to find any guide on how to upstream completion files to
> zsh, so I decided to send them to the list, will be glad if anyone could
> point what to do to get them imported into the tree.

Posting patches to the mailing list is the right way to get them
imported.

It also helps if they follow our conventions on things like
indentation and naming. There is a guide on that in the file
Etc/completion-style-guide within the zsh distribution.

> Completion scripts were submitted to the got mailing list, but it was
> decided to talk to the zsh upstream first.

There are advantages to including completions with upstream projects.
They're more likely to be updated as new features are added and get
to follow got's releases with users of an old version of got getting
matching aged completions.

> Completion files for got, gotadmin and tog are inlined below.

We typically would use a single function for such cases and check
$service to determine which command we are completing for. Often there
are commonalities and this often facilitates sharing code for completion
of the separate commands. The function would normally be named similarly
to the project name or OS package for it. I'm not sure whether that
would be _got or _gameoftrees in this case. For example, _subversion
completes for both svn and svnadmin.

> +#compdef got
> +
> +_got_branches() {
> +	local branches

Our convention is 2 spaces for indentation.

> +        branches=$(got br -l 2>/dev/null | awk '{print $(NF-1)}' | sed 's/:$//')

By writing these sort of things as nested zsh substitutions, you avoid
forks which makes them quite a bit faster. I'll admit that those can be
fairly unpleasant to read so don't consider that to be essential.

> +        reply=(${(f)branches})
> +}
> +
> +_got_branch_name() {

Convention is for plural names on helper functions that complete things.

> +	local -a candidates
> +	local name
> +
> +	_got_branches
> +	for name in $reply; do
> +		candidates+=("$name")
> +	done

Isn't this just the same as doing:
  candidates=( $reply )
or later doing compadd -a reply

It can be helpful to provide a description. If you don't have them
enabled you may not see them but many users do enable descriptions. In
this case, it would be something like:

  local -a expl
  _description branches expl 'branch'
  compadd "$@" "$expl[@]" -a reply

> +_got_worktree_repository_path() {
> +	local dir=$PWD
> +
> +	reply=()
> +	while true; do
> +		if [[ -f $dir/.got/repository ]]; then

There's probably simpler ways to do this such as

  reply=( (../)#.got/repository(NY1.) )

Change the last . to a / if it is a directory rather than a plain file.

But if got can return it that can have advantages such as support for
any equivalent to $GIT_CEILING_DIRECTORIES.

> +_got_branches_or_keywords() {

Where you've got a mix of things like this, they'll want different
descriptions and many people configure them to be grouped separately.
Look for examples of _alternatives.

> +_got_stage_files() {
> +	_got_stage_paths
> +	compadd -a reply
> +}

Does got status list files in subdirectories? I suspect this isn't going
to work much like normal file completion does. As a minimum you want to
use the -f option to compadd when completing files but you may want to
look at other similar functions like _subversion and _git, not that they
are perfect either.

> +_got_init() {

For subcommands, the naming convention would be _got-init

> +	args)

At this stage, $curcontext should be updated to include the subcommand.
There are lots of examples of this, usually something like:

  curcontext=${curcontext%:*}-$line[1]:

> +		case $line[1] in
> +		info)
> +			_got_info
> +			;;

It's a good idea to match * at the end of the case statement and call
_default for it. That way if a new subcommand is added but not supported
by the completion, users will at least get a working default of filename
completion

Thanks

Oliver