Page MenuHomePhabricator

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

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

Details

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
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!
epriestley requested review of this revision.Sep 21 2018, 10:45 PM
epriestley updated this revision to Diff 47056.
  • More-tested version which removes ZSH support since it doesn't work.
amckinley accepted this revision.Sep 22 2018, 2:00 AM

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.

Prune .gitignore slightly more carefully.

This revision was automatically updated to reflect the committed changes.