Skip to content
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

Complete list of available scripts for npm run/run-script command #2366

Closed
wants to merge 5 commits into from
Closed

Conversation

stefanmaric
Copy link
Contributor

It will complete available scripts for npm run/npm run-script command in current project.

screenshot from 2015-09-07 10-38-16

@stefanmaric
Copy link
Contributor Author

Aside: it should be considered to update the whole npm.fish file to use npm's completion tool; I would like to do it, but I have not enough knowledge about fishshell.

npm completion for bash/zsh directly from npm completion's output:

#
# Installation: npm completion >> ~/.bashrc  (or ~/.zshrc)
# Or, maybe: npm completion > /usr/local/etc/bash_completion.d/npm
#

COMP_WORDBREAKS=${COMP_WORDBREAKS/=/}
COMP_WORDBREAKS=${COMP_WORDBREAKS/@/}
export COMP_WORDBREAKS

if type complete &>/dev/null; then
  _npm_completion () {
    local si="$IFS"
    IFS=$'\n' COMPREPLY=($(COMP_CWORD="$COMP_CWORD" \
                           COMP_LINE="$COMP_LINE" \
                           COMP_POINT="$COMP_POINT" \
                           npm completion -- "${COMP_WORDS[@]}" \
                           2>/dev/null)) || return $?
    IFS="$si"
  }
  complete -o default -F _npm_completion npm
elif type compdef &>/dev/null; then
  _npm_completion() {
    local si=$IFS
    compadd -- $(COMP_CWORD=$((CURRENT-1)) \
                 COMP_LINE=$BUFFER \
                 COMP_POINT=0 \
                 npm completion -- "${words[@]}" \
                 2>/dev/null)
    IFS=$si
  }
  compdef _npm_completion npm
elif type compctl &>/dev/null; then
  _npm_completion () {
    local cword line point words si
    read -Ac words
    read -cn cword
    let cword-=1
    read -l line
    read -ln point
    si="$IFS"
    IFS=$'\n' reply=($(COMP_CWORD="$cword" \
                       COMP_LINE="$line" \
                       COMP_POINT="$point" \
                       npm completion -- "${words[@]}" \
                       2>/dev/null)) || return $?
    IFS="$si"
  }
  compctl -K _npm_completion npm
fi
###-end-npm-completion-###

@faho
Copy link
Member

faho commented Sep 7, 2015

Aside: it should be considered to update the whole npm.fish file to use npm's completion tool; I would like to do it, but I have not enough knowledge about fishshell.

What does that thing actually do? The documentation is a bit sparse.

Does it receive a full commandline (like e.g. "npm run-script ") and then offer all possibilities that can result from that? If that's the case, something like complete -c npm -a "(npm completion -- (commandline -oc))" should suffice.

@stefanmaric
Copy link
Contributor Author

Hi @faho,

I'm not sure, but I will dig into it and maybe come with another pull-request.

I'm just concerned about loosing the command and command options descriptions with that approach. I will read more about fish completions.

@ghost
Copy link

ghost commented Sep 7, 2015

@faho This PR adds completions for scripts defined in your local project's package.json

  "name": "my-node-library",
  "version": "1.0.0",
  "description": "Description goes here.",
  "main": "index.js",
  "scripts": {
    "test": "npm run tap",
    "tap": "NODE_ENV=development tap test.js"
  },

@stefanmaric Good job, but more useful would be the script and the value.

The following does it, feel free to readapt and PR again.

command npm run | command tail -n +2 | command sed "s/^ *//" | while read -l name
  read -l value
  complete -c npm -n "__fish_npm_using_command run" -a (printf "%s " "$name") -d "$value"
end

x

@stefanmaric
Copy link
Contributor Author

@bucaran I implemented your code. I had to add the -f option to complete in oder to get a similar output.

Aside: I noticed that I have quite long npm scripts, fish seems to trim descriptions to fit terminal viewport, still hard to scan my scripts:

screenshot from 2015-09-07 15-30-51

Any suggestions on how to improve this? I might want to trim script value to a fixed length.

@stefanmaric
Copy link
Contributor Author

In other hand, I was trying to translate npm completion-provided completion script for bash&zsh into fish completion with little success. I achieved to set the env variables as npm completion would expect but it crashes trying to access length on an undefined variable, presumingly a missing argument: npm completion -- "${COMP_WORDS[@]}"

I have no idea how to translate that little piece into fish, I expected it to simply be the whole command, but I tried and it didn't work.

@ghost
Copy link

ghost commented Sep 7, 2015

Any suggestions on how to improve this? I might want to trim script value to a fixed length.

Just cut value.

command npm run | command tail -n +2 | command sed "s/^ *//" | while read -l name
  set -l trim 10
  read -l value
  complete -c npm -n "__fish_npm_using_command run" \
    -a (printf "%s " "$name") -d (echo "$value" | cut -c1-$trim)
end

I have no idea how to translate that little piece into fish

This probably. Also check out this bash/fish translation table.

bash fish
"${COMP_WORDS[@]}" $COMP_WORDS

@stefanmaric
Copy link
Contributor Author

Awesome @bucaran! I was just not sure if it was a great idea, but with your approval, there it is.

I will keep digging if a completion using npm completion is viable and open another pull-request if so.

Thanks!

@ghost
Copy link

ghost commented Sep 7, 2015

I was just not sure if it was a great idea

There is probably an easier way 😅

@faho I don't know about hardcoding the trim factor, but in general LGTM.

@faho
Copy link
Member

faho commented Sep 7, 2015

"${COMP_WORDS[@]}" | $COMP_WORDS

Yes, and no - while ${VAR[@]} is bash syntax for "all values of the array $VAR", which can be replaced with a simple $VAR in fish, the variable "COMP_WORDS" has a certain meaning in bash's completion system - it's "An array variable consisting of the individual words in the current command line" (from here). In fish's system, that'd be commandline -opc (though I've never checked what "-p" actually does). @stefanmaric: You might want to read that link, it explains all those weird variables bash's completion system uses. For fish, check the complete and commandline docs.

@faho I don't know about hardcoding the trim factor, but in general LGTM.

My assumption is that the npm complete stuff would obsolete all of this, so I'd rather we check that before merging this.

@ghost
Copy link

ghost commented Sep 7, 2015

@faho Do you mean finding a way to reuse the npm complete bash script?

@stefanmaric
Copy link
Contributor Author

My assumption is that the npm complete stuff would obsolete all of this, so I'd rather we check that before merging this.

I think so, only if it is suitable for fishshell (I'm not sure we can provide commands and options descriptions using npm complete and not writing much of the code npm.fish already has, because it pretty much is designed for bash). So I would merge this in the meanwhile.

I have been busy with some other stuff today and haven't had chance to dig deeper.

@faho
Copy link
Member

faho commented Sep 8, 2015

Try this:

function __fish_complete_npm
    set -lx COMP_LINE (commandline -o)
    set -lx COMP_CWORD (count (commandline -c))
    set -lx COMP_POINT (expr length (commandline -c))
    npm completion -- $COMP_LINE ^/dev/null
end

complete -f -c npm -a "(__fish_complete_npm)"

This is a bit fiddly, but it almost works. The main issue is it's rather hard to detect if the cursor is directly after the last word (i.e. something like npm config_ - the underline showing the cursor position) or on the next currently empty word (i.e. npm config _). This means COMP_CWORD will be off by one, meaning npm will try to complete the last already completed word.

@faho
Copy link
Member

faho commented Sep 8, 2015

This seems to work:

function __fish_complete_npm
    set -lx COMP_LINE (commandline -o)
    set -lx COMP_CWORD (count (commandline -c))
    set -lx COMP_POINT (expr length (commandline -c))
    # If the cursor is after the last word, the empty token won't be counted by `count`
    if test (commandline -ct) = ""
        set COMP_CWORD (expr $COMP_CWORD + 1)
        set COMP_LINE $COMP_LINE ""
    end
    npm completion -- $COMP_LINE ^/dev/null
end

complete -f -c npm -a "(__fish_complete_npm)"

@faho
Copy link
Member

faho commented Sep 8, 2015

Sorry, this one should work, please test!

function __fish_complete_npm
    set -lx COMP_LINE (commandline -o)
    # bash starts arrays with 0
    set -lx COMP_CWORD (expr (count $COMP_LINE) - 1)
    set -lx COMP_POINT (expr length "$COMP_LINE")
    # If the cursor is after the last word, the empty token will disappear in the expansion
    if test (commandline -ct) = ""
        set COMP_CWORD (expr $COMP_CWORD + 1)
        set COMP_LINE $COMP_LINE ""
    end
    npm completion -- $COMP_LINE ^/dev/null
end

complete -f -c npm -a "(__fish_complete_npm)"

Now, even if we do this we should probably keep some hand-rolled completions for the descriptions, but everything without one should go.

@faho
Copy link
Member

faho commented Sep 8, 2015

Another small bug - when the cursor isn't at the end of the commandline, COMP_POINT was off:

function __fish_complete_npm --description "Complete the commandline using npm's 'completion' tool"
    # npm completion is bash-centric, so we need to translate fish's "commandline" stuff to bash's $COMP_* stuff
    # COMP_LINE is an array with the words in the commandline
    set -lx COMP_LINE (commandline -o)
    # COMP_CWORD is the index of the current word in COMP_LINE
    # bash starts arrays with 0, so subtract 1
    set -lx COMP_CWORD (expr (count $COMP_LINE) - 1)
    # COMP_POINT is the index of point/cursor when the commandline is viewed as a string
    set -l cmd_to_point (commandline -oc)
    set -lx COMP_POINT (expr length "$cmd_to_point")
    # If the cursor is after the last word, the empty token will disappear in the expansion
    # Readd it
    if test (commandline -ct) = ""
        set COMP_CWORD (expr $COMP_CWORD + 1)
        set COMP_LINE $COMP_LINE ""
    end
    npm completion -- $COMP_LINE ^/dev/null
end

complete -f -c npm -a "(__fish_complete_npm)"

If you're unsure how to test: Place that in "~/.config/fish/completions/npm.fish".

Now my main concern is that I'm not sure if expr is everwhere - it's in GNU coreutils, but I'm unsure about OSX. Once the string stuff (#2296) is merged, this could be replaced with some of that and math, so it's not too big of an issue.

@ghost
Copy link

ghost commented Sep 8, 2015

@faho expr is in OSX too.

@faho
Copy link
Member

faho commented Sep 8, 2015

expr is actually unnecessary because the string length stuff isn't actually string length stuff - it's "cursor position in the buffer" stuff, and that can be done with a simple commandline -C.

So my current version is:

function __fish_complete_npm --description "Complete the commandline using npm's 'completion' tool"
    # npm completion is bash-centric, so we need to translate fish's "commandline" stuff to bash's $COMP_* stuff
    # COMP_LINE is an array with the words in the commandline
    set -lx COMP_LINE (commandline -o)
    # COMP_CWORD is the index of the current word in COMP_LINE
    # bash starts arrays with 0, so subtract 1
    set -lx COMP_CWORD (math (count $COMP_LINE) - 1)
    # COMP_POINT is the index of point/cursor when the commandline is viewed as a string
    set -lx COMP_POINT (commandline -C)
    # If the cursor is after the last word, the empty token will disappear in the expansion
    # Readd it
    if test (commandline -ct) = ""
        set COMP_CWORD (math $COMP_CWORD + 1)
        set COMP_LINE $COMP_LINE ""
    end
    npm completion -- $COMP_LINE ^/dev/null
end

complete -f -c npm -a "(__fish_complete_npm)"

From my (limited) testing, it seems we get the run stuff for free with this, and fish is smart enough to coalesce duplicate completion options into one with a description, so I actually rather like this.

@stefanmaric
Copy link
Contributor Author

@faho yes it works for me. And has some nice features not available so far with npm.fish like listing installed packages for npm remove/ls/unlink/update and the npm run script suggestions that this pull-request is all about.

Not so good: option completion is poor. It suggest ALL options for ALL commands and doesn't distinct between long and short option arguments:

screenshot from 2015-09-08 11-10-11
screenshot from 2015-09-08 11-17-25
screenshot from 2015-09-08 11-12-13

npm actually kinda treats both as the same, --g acts just like -g and -save-dev acts like --save-dev; but the output is long and confusing that way.

And of course no descriptions. I believe it would still need all the code npm.fish already has.

I think that implementing npm complete brings more cons than pros, at least in this way. It might be included, for example, to suggest installed packages. (the only feature I can point that fish's completion hasn't yet).

@faho
Copy link
Member

faho commented Sep 8, 2015

And of course no descriptions. I believe it would still need all the code npm.fish already has.

Most of it. As far as I can see, we can currently only remove __fish_npm_settings and the help completion (the -a "$npm_cmds" one), but this is more extensible - as soon as npm completion is improved fish users will profit from it, we get completions that are bound to the npm version automatically. They aren't quite as good as hand-rolled ones because of the descriptions, but it's better to have an option without a description than no option at all. Also we get free run and remove completion like you said.

I think that implementing npm complete brings more cons than pros, at least in this way.

The major issue here is that the option completion is poor. Excepting upstream improvement (which would benefit all npm users), there's a few things we could do:

  • Not use npm completion - this means implementing run-script and such on our own - my assumption is that there's more we haven't seen
  • Use npm completion conditionally - i.e. use it as the catch-all function to provide options for run, remove etc - this would be doable, and still a lot less code/work than implementing it all in fish
  • Use npm completion except for options - i.e. add a switch/case (or string match once that's in) to abort completion if the current token starts with a "-" - assuming only the option completions are poor, this would result in less code than the second option while providing the same benefits

What do you think?

@stefanmaric
Copy link
Contributor Author

@faho I was also thinking in the third option you propose, I was testing around but got weird behavior with the npm run thing:

screenshot from 2015-09-08 12-34-49

Using:

function __fish_npm_needs_option
  set -l DASH (commandline -ct)
  if test $DASH != "-" -a $DASH != "--"
    return 0
  end

  return 1
end

EDIT: sorry about that non-sense function. I was expecting a comandline -n inverted option. I should have called it __fish_npm_does_not_need_option.

and:

function __fish_complete_npm --description "Complete the commandline using npm's 'completion' tool"
  # npm completion is bash-centric, so we need to translate fish's "commandline" stuff to bash's $COMP_* stuff
  # COMP_LINE is an array with the words in the commandline
  set -lx COMP_LINE (commandline -o)
  # COMP_CWORD is the index of the current word in COMP_LINE
  # bash starts arrays with 0, so subtract 1
  set -lx COMP_CWORD (math (count $COMP_LINE) - 1)
  # COMP_POINT is the index of point/cursor when the commandline is viewed as a string
  set -lx COMP_POINT (commandline -C)
  # If the cursor is after the last word, the empty token will disappear in the expansion
  # Readd it
  if test (commandline -ct) = ""
    set COMP_CWORD (math $COMP_CWORD + 1)
    set COMP_LINE $COMP_LINE ""
  end
  npm completion -- $COMP_LINE ^/dev/null
end

complete -f -c npm -n '__fish_npm_needs_option' -a "(__fish_complete_npm)"

and:

command npm run | command tail -n +2 | command sed "s/^ *//" | while read -l name
  set -l trim 20
  read -l value
  complete -f -c npm -n "__fish_npm_using_command run" -a (printf "%s " "$name") -d (echo "$value" | cut -c1-$trim)
  complete -f -c npm -n "__fish_npm_using_command run-script" -a (printf "%s " "$name") -d (echo "$value" | cut -c1-$trim)
end

And I opened an issue (npm/npm#9524), but look at what they answered me. :c

@faho
Copy link
Member

faho commented Sep 8, 2015

function __fish_npm_needs_option
set -l DASH (commandline -ct)
if test $DASH != "-" -a $DASH != "--"
return 0
end

return 1
end

Umh, you probably want some kind of pattern matching for that - currently the only way fish exposes it is via switch/case:

function __fish_npm_needs_option
    switch (commandline -ct)
        case "-*"
          return 0
    end
    return 1
end

complete -f -c npm -n '__fish_npm_needs_option' -a "(__fish_complete_npm)"

That's the wrong way around, no? This only uses npm completion if it needs an option (i.e. the current token is an option).

And I opened an issue npm/npm#9524, but look at what the answered me. :c

That's.... not great.

@stefanmaric
Copy link
Contributor Author

Umh, you probably want some kind of pattern matching for that - currently the only way fish exposes it is via switch/case:

Sweet! That's way prettier... But...

That's the wrong way around, no? This only uses npm completion if it needs an option (i.e. the current token is an option).

I'm sorry that my code is so confusing. I used test $DASH != "-" -a $DASH != "--" to return 0, so it does the opposite of what function name means. I was searching for something like -n '!__fish_npm_needs_option'. Should I simply rename the function? __fish_npm_doesnt_need_option(?)

Any idea why some completions offered on npm run lack the description? When commenting out the npm completion code, it works.

@faho
Copy link
Member

faho commented Sep 8, 2015

I was searching for something like -n '!__fish_npm_needs_option'. Should I simply rename the function? __fish_npm_doesnt_need_option(?)

You're looking for not, as in complete -n "not __fish_npm_needs_option". That alternative name is quite ugly.

Any idea why some completions offered on npm run lack the description? When commenting out the npm completion code, it works.

Are all of them still offered? If so, that's probably a bug in fish proper. If not, that's to be expected, the other code doesn't offer all possibilities so there's no descriptions for the ones it doesn't.

@stefanmaric
Copy link
Contributor Author

You're looking for not, as in complete -n "not __fish_npm_needs_option". That alternative name is quite ugly.

Damn, dummy me! I forgot everything in fish is a function and not is a function. That's what I needed. Thanks!

Are all of them still offered?

Yes, they are all offered. That was the first thing I checked.

EDIT:

Just commenting:

#complete -f -c npm -n 'not __fish_npm_needs_option' -a "(__fish_complete_npm)"

Works:
screenshot from 2015-09-08 15-05-29

I'm going to commit anyway in case you guys want to take a look.

@faho
Copy link
Member

faho commented Sep 8, 2015

Humm... maybe there's an issue with sorting?

Otherwise, could you remove __fish_npm_settings and other things without description? They are just useless cruft now. The rest looks good.

@stefanmaric
Copy link
Contributor Author

could you remove __fish_npm_settings and other things without description? They are just useless cruft now.

Totally forgot. There it is now.

Humm... maybe there's an issue with sorting?

I found another case for that weird behavior:

screenshot from 2015-09-08 17-07-14

Notice how add-user lacks description even though it has been declared:

complete -f -c npm -n '__fish_npm_needs_command' -a 'adduser add-user login' -d 'Add a registry user account'

And it does appear in this case:
screenshot from 2015-09-08 17-07-23

@faho
Copy link
Member

faho commented Sep 8, 2015

I found another case for that weird behavior:

I can reproduce that, but I can't see how this is the fault of this completion script - it seems to be a bug in fish-proper. So.. are you okay with me merging this, or do you want to work on it some more?

read -l value
complete -f -c npm -n "__fish_npm_using_command run" -a (printf "%s " "$name") -d (echo "$value" | cut -c1-$trim)
complete -f -c npm -n "__fish_npm_using_command run-script" -a (printf "%s " "$name") -d (echo "$value" | cut -c1-$trim)
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stefanmaric How about rewriting those two lines to?

for cmd in run run-script
  complete -f -c npm -n "__fish_npm_using_command $cmd" -a (printf "%s " "$name") -d (echo "$value" | cut -c1-$trim)
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, wait a minute... I don't think that works like you think.

That command npm run | ... stuff will be executed once, when the completion is auto-loaded! (Append echo "npm completions loaded" >&2 to npm.fish to see it)

Now, if I understand correctly, npm run is dependent on the current directory, so it'll trim and complete only the first directory you call the completion in (for a given shell instance).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be something like

function __fish_npm_run
    command npm run | command tail -n +2 | command sed "s/^ *//" | while read -l name
        set -l trim 20
        read -l value
        echo "$value" | cut -c1-$trim | read -l value
        printf "%s\t%s\n" $name $value
    end
end

for c in run run-script
    complete -f -c npm -n "__fish_npm_using_command $c" -a "(__fish_npm_run)"
end

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the completion is auto-loaded!

Good call @faho

Now, if I understand correctly, npm run is dependent on the current directory, so it'll trim and complete only the first directory you call the completion in (for a given shell instance).

This is correct.

⬆︎ LGTM

@stefanmaric Ping 😄

@stefanmaric
Copy link
Contributor Author

Hey guys, I pushed the fix for npm run completion. I replaced tail with grep because npm run sometimes puts messages and empty lines in between.

I think it is good now except for that bug with some descriptions present and some not.

@stefanmaric
Copy link
Contributor Author

@faho probably you already got into this, but I will share it anyway:

I created a dummy completion:

# ~/.config/fish/completions/hue.fish
complete -f -c hue -a "run"
complete -f -c hue -a "run" -d "RUN FOREST, RUN!"

complete -f -c hue -a "reveal" -d "SHOW ME BABY!"
complete -f -c hue -a "reveal"

Result:

screenshot from 2015-09-09 11-35-15

EDIT: misspelled Forrest, lol.

So if fishshell is meant to merge completions, yes, it is failing.

Using fish, version 2.2.0-227-g8bf1e69.

@ghost
Copy link

ghost commented Sep 9, 2015

@stefanmaric Edge case. But yes, it would be nice if instead of overwriting the last completion it would some how "merge" them.

@stefanmaric
Copy link
Contributor Author

That @bucaran, I actually saw @faho commenting on this very same thread:

fish is smart enough to coalesce duplicate completion options into one with a description

I still don't get why, in npm.fish case, some are overwritten and some not; if both functions were called asynchronously, I would expect a race condition and a variable output, but I always get the same result.

@faho
Copy link
Member

faho commented Sep 9, 2015

fish is smart enough to coalesce duplicate completion options into one with a description

Unfortunately, it appears it isn't. "assume", "ass", "u", "me", and all that.

I'm going to try to play with it some more - it appears that it's no fault of this completion script, but maybe there's a workaround to be found (in e.g. reordering).

@faho
Copy link
Member

faho commented Sep 10, 2015

Okay, I've played with this some more, and ordering definitely has an impact - if I put __fish_complete_npm (and the corresponding complete call) after everything else it suddenly describes add-user but lacks description for "home". In other words, it changes the situation but doesn't improve it. I've also hacked in a pipe to sort for npm completion and it also changes things, but again without clearly improving.

I still believe this is a bug in fish - at the least in the case where one call offers a description and another doesn't, it should pick the description.

Anyway, regarding this script, we have a couple of options:

  • Remove the npm completion stuff again - since it actually offers additional options (one is the "access" command), I'm not a fan of this
  • Blacklist the npm completion stuff for even more - essentially everything we have descriptions for - this is probably more work than we think, and even harder to do correctly
  • Do some really evil hacking to generate both kinds of completions and filter them manually - this is probably a complete rewrite of the script
  • Ignore it here, fix the bug/change behavior where it should be, and ship this as-is

Frankly, everything but the last looks bad to me.

# and: https://github.com/fish-shell/fish-shell/pull/2366
complete -f -c npm -n 'not __fish_npm_needs_option' -a "(__fish_complete_npm)"

# list available npm scripts and their parial content
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: Partial

@faho faho self-assigned this Sep 11, 2015
@faho
Copy link
Member

faho commented Sep 12, 2015

Merged now, thanks!

Let's see what upstream does with the "completion" tool - it's a nice idea.

@faho faho closed this Sep 12, 2015
@faho faho added this to the next-2.x milestone Sep 26, 2015
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants