-
Notifications
You must be signed in to change notification settings - Fork 125
ENT-248: Completion installation #180
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
Conversation
This reverts some of the changes from commit 314004c.
…` command. The curl|bash installer calls `entire curl-bash-post-install` after installing the binary, so the interactive completion prompt belongs there rather than in `entire enable`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 2ea0c2a8161d
…ionNonInteractive. Add `autoload -Uz compinit && compinit` before the zsh completion source line so completions work out of the box. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: ffd2a5cf8a87
Entire-Checkpoint: 818cdec7d352
Entire-Checkpoint: 818cdec7d352
…status. Print a message when completions are already configured or when the user's shell isn't supported, rather than exiting silently. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: b376aca4c326
This reverts commit d600013. We'll deal with this in a separate PR.
Fish is weird and different, never mind for now. This reverts commit bb58744.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR moves shell-completion setup out of entire enable and into a post-install flow, aiming to improve installation ergonomics (especially for Homebrew and the curl install.sh | bash path).
Changes:
scripts/install.shnow invokes a hiddenentire curl-bash-post-installsubcommand after installing the binary.- Removed
--setup-shellbehavior fromentire enableand refactored completion targeting logic intoshellCompletionTarget(). - Release pipeline now generates and ships
completions/*artifacts, and wires them into the Homebrew packaging config.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/install.sh | Runs a new post-install hook via the freshly installed entire binary. |
| cmd/entire/cli/setup.go | Removes enable-time completion setup; adds post-install command and refactors shell completion logic. |
| cmd/entire/cli/setup_test.go | Updates tests to match the changed runEnableWithStrategy signature. |
| cmd/entire/cli/root.go | Registers the new hidden curl-bash-post-install command. |
| .goreleaser.yaml | Generates completion files during release and includes them in archives/Homebrew config. |
|
This is related to ENT-248 but isn't a fix, yet. There will be followups. |
shellCompletionTarget now returns an error instead of empty strings, allowing callers to distinguish between unsupported shells and home directory lookup failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: 42527c9c20c8
Entire-Checkpoint: 42527c9c20c8
Entire-Checkpoint: 42527c9c20c8
Entire-Checkpoint: 42527c9c20c8
Entire-Checkpoint: 42527c9c20c8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Entire-Checkpoint: dd9f29bc2a1a
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 10dd2cdb2cdd
Description
We're trying to make the onboarding as smooth as possible.
curl-bash-post-installaction, which will attempt to set up shell completionsThere are a lot of trade offs here and i don't know if we can achieve perfection. I couldn't think of a nice, robust way to ensure the PATH stuff, while remaining friendly and "one-shot" from the user's perspective. And between shell completions and being on
PATH, clearlyPATHis highest priority.So, if the user did not have a
~/.local/binon their PATH, we make sure that gets done. But, in the happy path case, we set up completions, too. On the bright side, if they later want to upgrade (which is justcurl | bashagain) we'll automatically guide them through the completions setup at that point.I'm open to feedback on this one but it's a tough set of tradeoffs. I think this is acceptable as is, and we can circle back if not.
Trivia
Along the way i discovered:
Worse: if we're telling users toOK, it turns outcurl|bash, the install script will always be running Bash. We should probably just ask users which shell they'd like to install completions for? Hmmmm...$SHELLis "special". It's set to your login shell's path, and if you invoke some other shell (say you're running Zsh and executebash) it won't be overridden. So it's an ok approach to use as we do.Note
Medium Risk
Touches the installation/onboarding path and modifies how the binary is discovered on
PATH, which can cause user-visible install failures or confusing behavior if edge cases are missed; changes are localized and covered by a small unit test for shell target selection.Overview
Improves onboarding for the
curl | bashinstaller by adding PATH guidance for first-time installs and detecting PATH conflicts, then running a new hidden CLI post-install command to optionally set up shell completions.Moves shell completion setup out of
entire enable(removes--setup-shelland related non-interactive logic) and refactors completion targeting intoshellCompletionTarget()with better bash rc-file selection (.bash_profilevs.bashrc) plus tests.Updates release packaging to generate and ship
completions/*in archives, and configures Homebrew cask to install bash/zsh/fish completions.Written by Cursor Bugbot for commit c269cb2. This will update automatically on new commits. Configure here.