Update of apm completions #2390

Merged
merged 1 commit into from Sep 16, 2015

Conversation

Projects
None yet
2 participants
@dan-c-underwood
Contributor

dan-c-underwood commented Sep 15, 2015

Completions for apm now support both the Advanced Power Management client and the Atom text editor package manager.

This addresses #2354

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Sep 15, 2015

Member

Have you tested with the power management thing?

Member

faho commented Sep 15, 2015

Have you tested with the power management thing?

@dan-c-underwood

This comment has been minimized.

Show comment
Hide comment
@dan-c-underwood

dan-c-underwood Sep 15, 2015

Contributor

Fixed the switch/case usage. Hopefully it looks better!

This does not work for "--help"

Which part is this point referencing? apm --help <command> is not valid and will just provide the apm help (which seems a bit silly given how much redundancy there seems to be for the command line tool!).

This does not work for per-command options - do those work (i.e. apm --hard uninstall $PACKAGE)?

Similar to above, any options written before the first command (except for --(no-)color) seem to be ignored.

Have you tested with the power management thing?

I'm downloading a Debian image at the moment with which I'll check in a VM to see if it breaks it for that, although it's a daemon I've never encountered!

Contributor

dan-c-underwood commented Sep 15, 2015

Fixed the switch/case usage. Hopefully it looks better!

This does not work for "--help"

Which part is this point referencing? apm --help <command> is not valid and will just provide the apm help (which seems a bit silly given how much redundancy there seems to be for the command line tool!).

This does not work for per-command options - do those work (i.e. apm --hard uninstall $PACKAGE)?

Similar to above, any options written before the first command (except for --(no-)color) seem to be ignored.

Have you tested with the power management thing?

I'm downloading a Debian image at the moment with which I'll check in a VM to see if it breaks it for that, although it's a daemon I've never encountered!

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Sep 15, 2015

Member

Which part is this point referencing? apm --help is not valid and will just provide the apm help (which seems a bit silly given how much redundancy there seems to be for the command line tool!).

Ah okay, I assumed "--help" was the same as "help".

Member

faho commented Sep 15, 2015

Which part is this point referencing? apm --help is not valid and will just provide the apm help (which seems a bit silly given how much redundancy there seems to be for the command line tool!).

Ah okay, I assumed "--help" was the same as "help".

@dan-c-underwood

This comment has been minimized.

Show comment
Hide comment
@dan-c-underwood

dan-c-underwood Sep 15, 2015

Contributor

This seems to be broken - it's case '*'

Whoops!

Contributor

dan-c-underwood commented Sep 15, 2015

This seems to be broken - it's case '*'

Whoops!

@dan-c-underwood

This comment has been minimized.

Show comment
Hide comment
@dan-c-underwood

dan-c-underwood Sep 15, 2015

Contributor

Tested with apmd, old completions still work fine (on Ubuntu 14.03).

Contributor

dan-c-underwood commented Sep 15, 2015

Tested with apmd, old completions still work fine (on Ubuntu 14.03).

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Sep 15, 2015

Member

Ah nice. I was afraid old-apm would print something to stdout or similar.

I've just managed to install atom (after about an hour of downloading/building, which seems a bit excessive), and the apm config completions are a bit wonky - they just repeat the next options over and over so you can get apm config get get get get set delete.

You might want to use something like apm config list | grep -v '^;\|^$' | tr "=" "\t" to list options to get/set or delete.

You might also want to take a look at the "npm" completions since apm is basically a wrapper around that (but don't try using "complete -w"). There doesn't seem to be much of use there.

Member

faho commented Sep 15, 2015

Ah nice. I was afraid old-apm would print something to stdout or similar.

I've just managed to install atom (after about an hour of downloading/building, which seems a bit excessive), and the apm config completions are a bit wonky - they just repeat the next options over and over so you can get apm config get get get get set delete.

You might want to use something like apm config list | grep -v '^;\|^$' | tr "=" "\t" to list options to get/set or delete.

You might also want to take a look at the "npm" completions since apm is basically a wrapper around that (but don't try using "complete -w"). There doesn't seem to be much of use there.

share/completions/apm.fish
+ complete -x -c apm -n '__fish_apm_config_args 2' -a delete -d 'Delete config item'
+ complete -x -c apm -n '__fish_apm_config_args 2' -a list -d 'List config items'
+ complete -x -c apm -n '__fish_apm_config_args 2' -a edit -d 'Edit config items'
+ complete -x -c apm -n '__fish_apm_config_args 3; and test (commandline -opc)[3] != "edit"; and test (commandline -opc)[3] != "list"' -a '(__fish_apm_config_items)'

This comment has been minimized.

@dan-c-underwood

dan-c-underwood Sep 15, 2015

Contributor

Is there a more efficient way of doing this?

@dan-c-underwood

dan-c-underwood Sep 15, 2015

Contributor

Is there a more efficient way of doing this?

This comment has been minimized.

@faho

faho Sep 15, 2015

Member

I would have probably split ...config_args into two functions - _fish_apm_config{needs_command,_using_command_from}.

What you could do is contains -- (commandline -opc)[3] edit list instead of the tests. That'd be a tiny bit shorter and more easily extensible. (Of course this is all assuming that config has no options that can appear before the subcommand - if there are and you missed them or if it ever changes, this code will stop being correct)

@faho

faho Sep 15, 2015

Member

I would have probably split ...config_args into two functions - _fish_apm_config{needs_command,_using_command_from}.

What you could do is contains -- (commandline -opc)[3] edit list instead of the tests. That'd be a tiny bit shorter and more easily extensible. (Of course this is all assuming that config has no options that can appear before the subcommand - if there are and you missed them or if it ever changes, this code will stop being correct)

@dan-c-underwood

This comment has been minimized.

Show comment
Hide comment
@dan-c-underwood

dan-c-underwood Sep 15, 2015

Contributor

Hold off on a merge, it's still missing the case apm --<option> config <command>.

Contributor

dan-c-underwood commented Sep 15, 2015

Hold off on a merge, it's still missing the case apm --<option> config <command>.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Sep 15, 2015

Member

Hold off on a merge

Don't worry, I almost merged something broken a few days ago, so I'm still paying attention 😆.

it's still missing the case apm -- config

Ah yes, that's the tough stuff. What I've recently done for git (not pushed yet as I want to test it thoroughly) is:

function __fish_git_needs_command
  set cmd (commandline -opc)
  if [ (count $cmd) -eq 1 ]
      return 0
  else
      set -l skip_next 1
      # Skip first word because it's "git" or a wrapper
      for c in $cmd[2..-1]
          test $skip_next -eq 0; and set skip_next 1; and continue
          # git can only take a few options before a command, these are the ones mentioned in the "git" man page
          # e.g. `git --follow log` is wrong, `git --help log` is okay (and `git --help log $branch` is superfluous but works)
          # In case any other option is used before a command, we'll fail, but that's okay since it's invalid anyway
          switch $c
              # General options that can still take a command
              case "--help" "-p" "--paginate" "--no-pager" "--bare" "--no-replace-objects" --{literal,glob,noglob,icase}-pathspecs  --{exec-path,git-dir,work-tree,namespace}"=*"
                  continue
              # General options with an argument we need to skip. The option=value versions have already been handled above
              case --{exec-path,git-dir,work-tree,namespace}
                  set skip_next 0
                  continue
              # General options that cause git to do something and exit - these behave like commands and everything after them is ignored
              case "--version" --{html,man,info}-path
                  return 1
              # We assume that any other token that's not an argument to a general option is a command
              case "*"
                  return 1
          end
      end
      return 0
  end
  return 1
end

(Yes, it's a bit long, but I hope it actually catches all the edgecases - maybe this could be generalized)

Member

faho commented Sep 15, 2015

Hold off on a merge

Don't worry, I almost merged something broken a few days ago, so I'm still paying attention 😆.

it's still missing the case apm -- config

Ah yes, that's the tough stuff. What I've recently done for git (not pushed yet as I want to test it thoroughly) is:

function __fish_git_needs_command
  set cmd (commandline -opc)
  if [ (count $cmd) -eq 1 ]
      return 0
  else
      set -l skip_next 1
      # Skip first word because it's "git" or a wrapper
      for c in $cmd[2..-1]
          test $skip_next -eq 0; and set skip_next 1; and continue
          # git can only take a few options before a command, these are the ones mentioned in the "git" man page
          # e.g. `git --follow log` is wrong, `git --help log` is okay (and `git --help log $branch` is superfluous but works)
          # In case any other option is used before a command, we'll fail, but that's okay since it's invalid anyway
          switch $c
              # General options that can still take a command
              case "--help" "-p" "--paginate" "--no-pager" "--bare" "--no-replace-objects" --{literal,glob,noglob,icase}-pathspecs  --{exec-path,git-dir,work-tree,namespace}"=*"
                  continue
              # General options with an argument we need to skip. The option=value versions have already been handled above
              case --{exec-path,git-dir,work-tree,namespace}
                  set skip_next 0
                  continue
              # General options that cause git to do something and exit - these behave like commands and everything after them is ignored
              case "--version" --{html,man,info}-path
                  return 1
              # We assume that any other token that's not an argument to a general option is a command
              case "*"
                  return 1
          end
      end
      return 0
  end
  return 1
end

(Yes, it's a bit long, but I hope it actually catches all the edgecases - maybe this could be generalized)

@dan-c-underwood

This comment has been minimized.

Show comment
Hide comment
@dan-c-underwood

dan-c-underwood Sep 15, 2015

Contributor

Thanks for that! I've been trying to figure out a way to filter out --<options> using sed but it's a bit fiddly! I'll have another go at it tomorrow with the example you've given and see if I can crack it.

Contributor

dan-c-underwood commented Sep 15, 2015

Thanks for that! I've been trying to figure out a way to filter out --<options> using sed but it's a bit fiddly! I'll have another go at it tomorrow with the example you've given and see if I can crack it.

@faho faho self-assigned this Sep 16, 2015

@dan-c-underwood

This comment has been minimized.

Show comment
Hide comment
@dan-c-underwood

dan-c-underwood Sep 16, 2015

Contributor

Quick update, I've managed to squish the ability to repeat subcommands (i.e. preventing apm publish minor minor minor minor) and to handle initial options (i.e. apm --color config get).

However I'm currently struggling to prevent duplication of the initial command (i.e. typing apm config config and then <tab> gives:

$ apm config config 
delete  (Delete config item)  get     (Get config item)  set  (Set config item)
edit     (Edit config items)  list  (List config items)

As you can't get to this via following the completions, is this something that's essential to avoid? I've tried to filter it out by checking to see whether the command (config) has appeared more than once from commandline but fish doesn't seem to want to set the variable properly!

This is the function at the moment:

function __fish_apm_using_command
    set cmd (commandline -opc)
    if test (count $cmd) -gt 1
        set -l command_seen 0
        for c in $cmd[2..-1]
            switch $c
                case '--color' '--no-color'
                    continue
                case $argv
                    if test command_seen -eq 0
                        set command_seen 1
                    else
                        return 1
                    end
                case '*'
                    return 1
            end
        end
        if test command_seen -eq 1
            return 0
        end
    end
    return 1
end

Can you see anything there that seems unreasonable or incorrect?

Contributor

dan-c-underwood commented Sep 16, 2015

Quick update, I've managed to squish the ability to repeat subcommands (i.e. preventing apm publish minor minor minor minor) and to handle initial options (i.e. apm --color config get).

However I'm currently struggling to prevent duplication of the initial command (i.e. typing apm config config and then <tab> gives:

$ apm config config 
delete  (Delete config item)  get     (Get config item)  set  (Set config item)
edit     (Edit config items)  list  (List config items)

As you can't get to this via following the completions, is this something that's essential to avoid? I've tried to filter it out by checking to see whether the command (config) has appeared more than once from commandline but fish doesn't seem to want to set the variable properly!

This is the function at the moment:

function __fish_apm_using_command
    set cmd (commandline -opc)
    if test (count $cmd) -gt 1
        set -l command_seen 0
        for c in $cmd[2..-1]
            switch $c
                case '--color' '--no-color'
                    continue
                case $argv
                    if test command_seen -eq 0
                        set command_seen 1
                    else
                        return 1
                    end
                case '*'
                    return 1
            end
        end
        if test command_seen -eq 1
            return 0
        end
    end
    return 1
end

Can you see anything there that seems unreasonable or incorrect?

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Sep 16, 2015

Member

Can you see anything there that seems unreasonable or incorrect?

That function seems okay. What you would probably need to do is only offer sub-subcommands to config if "__fish_apm_using_command config" is true (and no sub-subcommand is there yet) - in the double-config case that would be false. Or are you doing that already and the function doesn't work? I can't see anything on a cursory glance (you could skip a test or two if you inverted command_seen to be 0 when true and 1 when false - then you could return it directly - also "command_seen_once" would be clearer, but neither of those are important).

Though...

As you can't get to this via following the completions, is this something that's essential to avoid?

I've just mentioned this an hour ago:

I'm of the opinion that completions shouldn't offer invalid arguments, but can do whatever they want when the user enters some manually - the program will then error out anyway. So... meh.

If you can easily fix it, do it. If not, it's not necessary.

Is there something more pressing left to do here?

Member

faho commented Sep 16, 2015

Can you see anything there that seems unreasonable or incorrect?

That function seems okay. What you would probably need to do is only offer sub-subcommands to config if "__fish_apm_using_command config" is true (and no sub-subcommand is there yet) - in the double-config case that would be false. Or are you doing that already and the function doesn't work? I can't see anything on a cursory glance (you could skip a test or two if you inverted command_seen to be 0 when true and 1 when false - then you could return it directly - also "command_seen_once" would be clearer, but neither of those are important).

Though...

As you can't get to this via following the completions, is this something that's essential to avoid?

I've just mentioned this an hour ago:

I'm of the opinion that completions shouldn't offer invalid arguments, but can do whatever they want when the user enters some manually - the program will then error out anyway. So... meh.

If you can easily fix it, do it. If not, it's not necessary.

Is there something more pressing left to do here?

@dan-c-underwood

This comment has been minimized.

Show comment
Hide comment
@dan-c-underwood

dan-c-underwood Sep 16, 2015

Contributor

It looks like it never enters the if block in case $argv.

case $argv
    if test command_seen_once -eq 1
        breakpoint
        set command_seen_once 0
    else
        return 1
    end

The breakpoint above never gets hit, it always goes into the else block (I've made the stylistic changes so that's why it'll be slightly different to the original!).

The function is being called as: __fish_apm_using_command config; and not __fish_apm_includes_subcommand $config_subcommands.

__fish_apm_includes_subcommand checks whether a subcommand is currently in the commandline buffer - so negation used to prevent offering of completions if a subcommand has already been given.

Contributor

dan-c-underwood commented Sep 16, 2015

It looks like it never enters the if block in case $argv.

case $argv
    if test command_seen_once -eq 1
        breakpoint
        set command_seen_once 0
    else
        return 1
    end

The breakpoint above never gets hit, it always goes into the else block (I've made the stylistic changes so that's why it'll be slightly different to the original!).

The function is being called as: __fish_apm_using_command config; and not __fish_apm_includes_subcommand $config_subcommands.

__fish_apm_includes_subcommand checks whether a subcommand is currently in the commandline buffer - so negation used to prevent offering of completions if a subcommand has already been given.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Sep 16, 2015

Member

Uhm.... there's a "$" missing. (test $command_seen_once)

Member

faho commented Sep 16, 2015

Uhm.... there's a "$" missing. (test $command_seen_once)

@dan-c-underwood

This comment has been minimized.

Show comment
Hide comment
@dan-c-underwood

dan-c-underwood Sep 16, 2015

Contributor

That would explain a lot!

Contributor

dan-c-underwood commented Sep 16, 2015

That would explain a lot!

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Sep 16, 2015

Member

After that you're done if I see this correctly. Could you squash this (make a new branch and do git merge --squash $oldbranch) and publish the new branch so I can merge it without all this review noise?

(I'd also like to have a slightly more informative first line in the commit message: "Complete atom package manager or apmd" or so)

Member

faho commented Sep 16, 2015

After that you're done if I see this correctly. Could you squash this (make a new branch and do git merge --squash $oldbranch) and publish the new branch so I can merge it without all this review noise?

(I'd also like to have a slightly more informative first line in the commit message: "Complete atom package manager or apmd" or so)

@dan-c-underwood

This comment has been minimized.

Show comment
Hide comment
@dan-c-underwood

dan-c-underwood Sep 16, 2015

Contributor

Yep that's fine, I'll comment the functions up a bit more in case they're useful for other completions in the future and then get that done.

Contributor

dan-c-underwood commented Sep 16, 2015

Yep that's fine, I'll comment the functions up a bit more in case they're useful for other completions in the future and then get that done.

Complete atom package manager or apmd
Completions for `apm` now support both the Advanced Power Management client and the Atom text editor.
@dan-c-underwood

This comment has been minimized.

Show comment
Hide comment
@dan-c-underwood

dan-c-underwood Sep 16, 2015

Contributor

First time doing a squash so apologies if that has ended up terrible!

Contributor

dan-c-underwood commented Sep 16, 2015

First time doing a squash so apologies if that has ended up terrible!

@faho faho merged commit 18a1163 into fish-shell:master Sep 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Sep 16, 2015

Member

Merged, thanks for the nice work and quick replies!

Member

faho commented Sep 16, 2015

Merged, thanks for the nice work and quick replies!

@dan-c-underwood

This comment has been minimized.

Show comment
Hide comment
@dan-c-underwood

dan-c-underwood Sep 16, 2015

Contributor

Thanks for your help!

On 16 Sep 2015, at 16:00, Fabian Homborg notifications@github.com wrote:

Merged, thanks for the nice work and quick replies!


Reply to this email directly or view it on GitHub.

Contributor

dan-c-underwood commented Sep 16, 2015

Thanks for your help!

On 16 Sep 2015, at 16:00, Fabian Homborg notifications@github.com wrote:

Merged, thanks for the nice work and quick replies!


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment