[Wilds] Update "arc shell-complete" for toolsets
ClosedPublic
Actions

Authored by epriestley on Sep 21 2018, 10:44 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13098: Plans: Arcanist toolsets and extensions

Commits

rARC50dfc9cc41f5: [Wilds] Update "arc shell-complete" for toolsets

Summary

Ref T13098. Major changes:

arc shell-complete now installs itself to your ~/.profile. Running arc shell-complete again will update the hook and update the completion rules.

Completion rules work for all toolsets, so you can install once and then autocomplete in arc, phage, etc.

This code supports other shells in theory, and I developed most of it with ZSH support next to the Bash support. However, while actually testing ZSH support, I couldn't get it to even slightly work and found myself falling down a very, very deep rabbit hole of ZSH being entirely written in bash script and ${0:a:h} being a legitimate script construction. The existing ZSH support comes from one guy in 2012 and also does not work for me on master, so I ultimately removed it. Open to restoring it but I wasn't able to figure it out in 10 minutes of Googling and I'm not convinced it's worth 11 minutes of Googling.

I left a few rough edges here with notes on how to improve/fix them, but the basics all work.

Test Plan

Ran arc shell-complete under various stages of ~/.profile, couldn't get it to do anything bad.
Ran arc lib<tab>, arc shell-complete --curr<tab>, etc. Got sensible suggestions and completions.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 20867
Build 28379: Run Core Tests

Event Timeline

epriestley created this revision.Sep 21 2018, 10:44 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 21 2018, 10:45 PM

Harbormaster failed remote builds in B20867: Diff 47055!

More-tested version which removes ZSH support since it doesn't work.

The existing ZSH support comes from one guy in 2012 and also does not work for me on master, so I ultimately removed it.

I wouldn't trust those damn dirty ZSH users either.

.gitignore
35–36	Can probably drop these while you're at it.
src/toolset/workflow/ArcanistShellCompleteWorkflow.php
576	`//TODO`
581	`//TODO`
587	Shouldn't all of these be handled in the main `executeWorkflow` by returning 0 there? Or is this a "let eventual complications in these methods bubble up automatically through `executeWorkflow` situation?

This revision is now accepted and ready to land.Sep 22 2018, 2:00 AM

epriestley marked 2 inline comments as done.Sep 22 2018, 6:22 PM

epriestley added inline comments.

.gitignore
35–36	Ah, yeah.
src/toolset/workflow/ArcanistShellCompleteWorkflow.php
587	Yeah, the latter. I imagine maybe we'll return some kind of list or object or something eventually and then do one echo + return code at top level (perhaps if this gets unit testing, for example). No specific plans for why we'd do that, but this seemed a little better/more readable than having multiple `echo "FILE\n"` lines in the code, I just didn't go quite as far as to pulling the side effects out completely and restoring them at top level. The "FILE" and "ARGUMENT" things are actually how this works "properly" right now -- the Bash script looks for "FILE" and calls some kind of Bash magic (`compgen -f -- <directory>`) to mean "complete files in current directory". This is weird, but in the realm of Bash scripting it seems pretty mild. Since ZSH completion theoretically is the same as Bash completion with the addition of some "zsh, work like Bash" incantation but maybe if/when we write Powershell completion or whatever it will make sense to do file completion in-process. "ARGUMENT" means "nothing can be completed" and should maybe be renamed or otherwise made more obvious.