Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 11 months ago by Sheridan Rawlins Modified:
6 years, 11 months ago Reviewers:
bradnelson, bradn CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, M-A Ruel, iannucci+depot_tools_chromium.org, ilevy-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Visibility:
Public. |
DescriptionAdded basic completion (the commands) for gclient.
To try it out, do
. $DEPOT_TOOLS/gclient_completion.sh
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=245134
Patch Set 1 #Patch Set 2 : Let complete use the defaults unless using the help command. #
Total comments: 11
Patch Set 3 : Added more comments and some use sed instead of grep with perlre. #Patch Set 4 : Fixed flags & added comments. #Patch Set 5 : untabify. #Patch Set 6 : removed log. #
Created: 6 years, 11 months ago
Messages
Total messages: 9 (0 generated)
https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh File gclient_completion.sh (right): https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh#ne... gclient_completion.sh:1: # Run from either local inclusion or /etc/bash_completions.d So I don't have "have" on my goobuntu system. I assume it does inside the bash_completion plumbing. This does work if I comment out this line. Maybe clarify how to use this without installing globally? Overall a little more context on what this is for and how to use install it, as bash_completion plumbing is probably not widely groked. https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh#ne... gclient_completion.sh:5: gclient -h | grep -P '(?<= (?:\x1b)\[32m)([\w\d]+)' -o It looks like these terminal codes can vary depending on if the "colorama" package is installed. Is that handled? Why is the ESC character optional? Or has my vague recollection of perl regexs failed me? Also grep on osx doesn't seem to support perl style regexes. Variants like gclient update aren't in the help list (FWIW). https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh#ne... gclient_completion.sh:13: # Passthrough to git completion Huh? https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh#ne... gclient_completion.sh:24: -*) : ignore options ;; This hangs for me when there are flags, should that happen? https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh#ne... gclient_completion.sh:34: if [[ "$command" != help || "$cword" -le 2 ]]; then Maybe a comment explaining this is so gclient help ... completes?
Hmm, the cost (of getting working on all scenarios & install dependencies) to benefit ratio on this seem low to me. I'm going to close this out and keep local - if I get a chance to make it more general or anyone clamors for it, I'll resurrect it. Thanks for your comments. https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh File gclient_completion.sh (right): https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh#ne... gclient_completion.sh:5: gclient -h | grep -P '(?<= (?:\x1b)\[32m)([\w\d]+)' -o On 2014/01/07 02:04:49, bradn wrote: > It looks like these terminal codes can vary depending on if the "colorama" > package is installed. > Is that handled? Why is the ESC character optional? Or has my vague recollection > of perl regexs failed me? > > Also grep on osx doesn't seem to support perl style regexes. > > Variants like gclient update aren't in the help list (FWIW). Added "update" manually, and used sed instead of grep - that should exist everywhere, right?
Message was sent while issue was closed.
I didn't mean to shut down landing this with too much scope. I'm fine with it landing in something like the current state if you're comfortable sticking a comment up top indicating it as requires some amount of setup. Maybe with a TODO to make it work out of the box? I've never tweaked my own bash completion setup, so I'm unsure if some amount of local config is typical. sed should be more portable.
Thanks for the nudge; spent a little longer with it and got it working on my mac (using fink's apt-get for a new sed and bash-completion). This is not intended to be needed by anything other than developers, so I added a few comments on what it needs and how to "install" it in .bashrc rather than/instead of /etc/bash_completion.d https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh File gclient_completion.sh (right): https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh#ne... gclient_completion.sh:1: # Run from either local inclusion or /etc/bash_completions.d Ok, I realize that installation into /etc/bash_completion.d is not as useful as sourcing directly in your .bashrc so you get the latest & greatest :D Removed this line. Also got working on my mac (using fink to apt-get bash-completion and a newer sed), and added comment to that effect. On 2014/01/07 02:04:49, bradn wrote: > So I don't have "have" on my goobuntu system. > I assume it does inside the bash_completion plumbing. This does work if I > comment out this line. > Maybe clarify how to use this without installing globally? > > Overall a little more context on what this is for and how to use install it, as > bash_completion plumbing is probably not widely groked. https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh#ne... gclient_completion.sh:13: # Passthrough to git completion On 2014/01/07 02:04:49, bradn wrote: > Huh? Improved comment - basically just aliased so that gclient fetch did the same thing as git fetch (if you've sourced the method (which I've also added a guard for). https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh#ne... gclient_completion.sh:24: -*) : ignore options ;; On 2014/01/07 02:04:49, bradn wrote: > This hangs for me when there are flags, should that happen? Wow copied from git-completion.bash, but forgot the ((c++)) line… Fixed. https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh#ne... gclient_completion.sh:34: if [[ "$command" != help || "$cword" -le 2 ]]; then On 2014/01/07 02:04:49, bradn wrote: > Maybe a comment explaining this is so gclient help ... completes? Done.
lgtm https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh File gclient_completion.sh (right): https://codereview.chromium.org/100463007/diff/40001/gclient_completion.sh#ne... gclient_completion.sh:5: gclient -h | grep -P '(?<= (?:\x1b)\[32m)([\w\d]+)' -o On 2014/01/16 01:12:59, Sheridan Rawlins wrote: > On 2014/01/07 02:04:49, bradn wrote: > > It looks like these terminal codes can vary depending on if the "colorama" > > package is installed. > > Is that handled? Why is the ESC character optional? Or has my vague > recollection > > of perl regexs failed me? > > > > Also grep on osx doesn't seem to support perl style regexes. > > > > Variants like gclient update aren't in the help list (FWIW). > > Added "update" manually, and used sed instead of grep - that should exist > everywhere, right? I think so.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scr@chromium.org/100463007/170001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scr@chromium.org/100463007/220001
Message was sent while issue was closed.
Change committed as 245134 |
||||||||||||||||||||||||||||||||||||||||||||||||||
