Backport: Add zsh support to completion script - #1562
Conversation
Make the existing bash completion script work in both bash and zsh
by auto-detecting the shell and adapting behavior accordingly:
- Add shell detection via $ZSH_VERSION / $BASH_VERSION
- Load bashcompinit in zsh for bash-style completion compatibility
- Use typeset instead of declare/compgen for zsh
- Use ${(P)var} instead of ${!var} for indirect expansion in zsh
- Accept 'zsh' as valid --completion argument in Completion.java
- Update README.adoc with zsh instructions
Based on the approach from PR #1514 by janweinschenker, consolidated
into the single existing script to avoid duplication.
Co-Authored-By: Jan Weinschenker <janweinschenker@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
The shell script changes are technically correct — typeset -f for function checks, typeset + for variable enumeration, ${(P)var} for indirect expansion, and bashcompinit are all standard zsh idioms. The single-script approach with runtime detection is clean.
Two observations:
-
Backport ahead of original (medium): This PR is described as a backport of #1559, but PR #1559 is still open on master with zero reviews. The commit 7f7db5e referenced as "master commit" actually lives on the
unified-bash-zsh-completionfeature branch, not master. Merging the backport before the original is reviewed risks divergence if the original gets modified during review. Consider merging #1559 on master first, then cherry-picking to mvnd-1.x. -
Missing zsh test (low): The existing
completionBash()test only exercises--completion bash. SinceCompletion.javanow acceptszsh, a parallelcompletionZsh()test callingclient.execute(output, "--completion", "zsh")would guard against regressions in argument validation.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Clean, well-structured backport adding zsh completion support. The auto-detection approach using bashcompinit is sound and CI passes.
Two minor items to consider:
-
Javadoc inconsistency in
Environment.java(medium): The Javadoc for theCOMPLETIONconstant (line 51) still reads "Only --completion bash is supported at this time", but this PR now accepts bothbashandzsh. The README correctly removes the bash-only language, butEnvironment.javawas not updated. This could mislead future maintainers. -
Missing zsh integration test (low):
CompletionNativeIT.javaonly tests--completion bash. Since bothbashandzshresolve to the same file and produce identical output, acompletionZsh()test would mainly guard the string validation gate inCompletion.java— nice to have but low priority.
Also note: PR #1559 (the original change on master) is still open. This makes this an unusual forward-backport of an unmerged change — you may want to merge #1559 first or note in the description that parallel merging is intentional.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
1 participant
Summary
Backport of #1559 to mvnd-1.x.
bashcompinitfor zsh compatibilitydeclare -F,compgen -v,${!var}) with shell-conditional equivalents (typeset -f,typeset +,${(P)var})bashandzshas--completionvaluesCherry-picked cleanly from master commit 7f7db5e.
🤖 Generated with Claude Code