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

Add pkgng command completions #2395

Closed
wants to merge 5 commits into from
Closed

Conversation

@xfix
Copy link
Member

@xfix xfix commented Sep 16, 2015

Fixes #1054.

@xfix xfix force-pushed the xfix:pkg-completions branch from f915500 to c8261bd Sep 16, 2015
Fixes #1054.
@xfix xfix force-pushed the xfix:pkg-completions branch from c8261bd to 3b053dd Sep 16, 2015
@faho

This comment has been minimized.

Copy link

@faho faho commented on share/completions/pkg.fish in 3b053dd Sep 16, 2015

This (__fish_use_subcommand) disallows using options with arguments before subcommands. I.e. something like pkg --rootdir /path/to/dir add. Does pkg accept that?

Note: We probably want something better than __fish_use_subcommand for these cases (which are one of the most common).

This comment has been minimized.

Copy link
Owner Author

@xfix xfix replied Sep 16, 2015

pkg --rootdir /path/to/dir add is allowed by pkg (never mind that it would just show help). However, interesting point you are bringing here, but I'm not sure exactly how this could be solved. (yum has a similar issue with commands like yum --installroot / install)

This comment has been minimized.

Copy link

@faho faho replied Sep 16, 2015

never mind that it would just show help

Does it just show help because pkgng detects it as invalid arguments or just because it's missing a package? In the former case we don't need to complete it. In the latter we should.

I'm not sure exactly how this could be solved

I previously advocated "__fish_seen_subcommand_from $commands", but that just checks if any of the words in $commands is a token anywhere in the commandline, which often doesn't quite match what the tool actually expects (in particular it can't know if an argument to an option looks like a command). In this case it seems reasonable, though and is relatively simple. What I did now for the git completions (not pushed yet, needs testing) is this:

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

which could possibly be generalized. Of course fish-shell#478 would solve this better.

This comment has been minimized.

Copy link
Owner Author

@xfix xfix replied Sep 16, 2015

I apologize for not being specific. The help message appears because the package wasn't specified. It's valid otherwise.

@faho

This comment has been minimized.

Copy link

@faho faho commented on share/completions/pkg.fish in 3b053dd Sep 16, 2015

I'm guessing pkg query "%n-%v" prints "$packagename-$packageversion". Is it necessary to specify the version or wouldn't it be nicer to display it as description instead of the redundant "Package"? In general I believe if every completion option belongs to the same category, it's unnecessary to print that. e.g. there's seldom a reason to use "File" as the description.

This comment has been minimized.

Copy link
Owner Author

@xfix xfix replied Sep 16, 2015

Hm, I agree that mentioning it's a package is pointless, I'm not sure why I did so.

@faho

This comment has been minimized.

Copy link

@faho faho commented on share/completions/pkg.fish in 3b053dd Sep 16, 2015

Isn't this an option also for "query"? Same thing for regex and glob.

This comment has been minimized.

Copy link
Owner Author

@xfix xfix replied Sep 16, 2015

Well, yes, the completions aren't really complete.

@faho

This comment has been minimized.

Copy link

@faho faho commented on share/completions/pkg.fish in 3b053dd Sep 16, 2015

How does this complete if the current token is empty (try pkng install <TAB>)? This also disables fuzzy completion by anchoring to the beginning of the name, making it inconsistent with other completions. (Which isn't too bad, just something to keep in mind)

This comment has been minimized.

Copy link
Owner Author

@xfix xfix replied Sep 16, 2015

Totally forgot about fuzzy completions, that was supposed to be purely performance enhancement, although considering pkg takes 0.4 seconds to generate the list, it may not be needed.

@faho
Copy link
Member

@faho faho commented Sep 16, 2015

I think we've noticed everything. Here's a list of my criticism (with obsolete points stricken):

  • Options-before-commands (pkg --rootdir $path add $package)
  • Redundant descriptions ("Package")
  • Incompleteness (can also be done later since this seems to cover most cases already)
  • Unnecessary version
  • Inhibiting fuzzyness

Merge when you feel it's okay - though I'd suggest squashing.

xfix added 2 commits Sep 16, 2015
@xfix
Copy link
Member Author

@xfix xfix commented Sep 16, 2015

@faho: Okay, I think I fixed the thing about "options-before-commands" (although option parsers are really complex).

(and thinking about it, I found a flaw in my code about code like -4c, but eh, this could be difficult to deal with within fish shell framework, and I don't think many people would do that)

@faho
Copy link
Member

@faho faho commented Sep 16, 2015

Yeah, this should work. If you're confident, merge!

@faho

This comment has been minimized.

Copy link
Member

@faho faho commented on share/completions/pkg.fish in eb7202e Sep 16, 2015

You're missing a "*" at the end - with this, it'll skip the next arg for "--rootdir=/path/to/dir". (Sorry, didn't know that was also an option

This comment has been minimized.

Copy link
Member

@faho faho replied Sep 16, 2015

"--=" is very likely to be invalid, and 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.

This comment has been minimized.

Copy link
Member Author

@xfix xfix replied Sep 16, 2015

--= is just as nonsensical as --iewigewigewgvbuwruhbdf=. I don't think a safety net is needed. Similarly, this completion gets confused by options like --reboot-computer-after-upgrade thinking it takes an argument when it doesn't even exist (the completion is done with wildcards because an user can technically shorten long options, for example use --ja instead of --jail).

@faho
Copy link
Member

@faho faho commented Sep 16, 2015

(and thinking about it, I found a flaw in my code about code like -4c, but eh, this could be difficult to deal with within fish shell framework, and I don't think many people would do that)

Depends on how pkg handles this - if it only recognizes arg-needing options as the last in the group (i.e. "-4c" is okay, "-c4" isn't), then you can just add another "*" - case -\*{o,j,c,r,C,R}

@xfix
Copy link
Member Author

@xfix xfix commented Sep 16, 2015

@faho: -c4 is okay, it's a single parameter, -c with argument 4, while -4c is two parameters, -4 and -c, and it awaits a value for -c.

Doing something like that would break stuff like -jjailc, as in this case c is not a parameter.

@xfix xfix closed this Sep 16, 2015
@xfix
Copy link
Member Author

@xfix xfix commented Sep 16, 2015

Merged with squash in any case (although I didn't notice your code comment until too late).

(it was a while since I did anything in fish shell)

@faho
Copy link
Member

@faho faho commented Sep 16, 2015

So it's like:

  • If an option group ends with an option that takes an argument, we need to skip the next arg
  • If an option that takes an arg is anywhere else, it takes the rest of the group as argument (or only the next letter)

Well, this is a bit quirky but should be doable.

@faho
Copy link
Member

@faho faho commented Sep 16, 2015

Merged with squash in any case (although I didn't notice your code comment until too late).

Yeah but you seem to have fixed it right away. Nice work!

@faho
Copy link
Member

@faho faho commented Sep 16, 2015

If an option group ends with an option that takes an argument, we need to skip the next arg

This should almost work just by adding another "*" between the "-" and the argument-taking shortopts like I commented above: case -\*{o,j,c,r,C,R}. The only part that wouldn't be working is if you have an argument-taking opt and an argument to that that looks like an argument-taking opt in the same group (e.g. "-cr"). I'd be okay with that since it seems like all argument-taking opts take directories, so the edge-case is only triggered if the user wants to use a directory named "r" or similar.

@xfix
Copy link
Member Author

@xfix xfix commented Sep 16, 2015

@faho: Option -j is a jail identifier.

(and by the same logic, it's unlikely the user will want to use even one option, not to say many of those global options, considering they are quite unusual options to use)

@faho
Copy link
Member

@faho faho commented Sep 16, 2015

Option -j is a jail identifier.

Which doesn't make much of a difference - or is "c" or "R" a typical jail identifier?

If you want, fix it, if not, okay.

@xfix
Copy link
Member Author

@xfix xfix commented Sep 16, 2015

@faho: No, but darthvader may be a jail identifier. -jdarthvader is fine.

@faho
Copy link
Member

@faho faho commented Sep 16, 2015

Yeah, you're right.

@zanchey zanchey added this to the next-2.x milestone Sep 25, 2015
@faho faho added the release notes label Oct 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
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.