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

Allow completers to disable unwanted shell behaviors #414

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

roberth
Copy link
Contributor

@roberth roberth commented Feb 23, 2021

This pr adds mkCompleterWithOptions :: (String -> IO [CompletionItem]) -> Completer, which allows the completer to disable unwanted shell behaviors like

  • adding a trailing slash to matches that coincide with directories
  • adding a trailing space while the completion is partial

The latter is useful for hierarchical path completions that aren't in the filesystem. Shell come with hacks to make the filesystem case work well, but you need to disable those if you need to implement your own path-like completions.

zsh and bash have facilities to override this behavior dynamically. zsh does so per match, whereas bash has parameters we can set per invocation, which seems to work equally well. fish doesn't have any of this and decides heuristically based on the last character.

A CompletionItem is essentially a tuple of a String and CompletionItemOptions, where

data CompletionItemOptions = CompletionItemOptions {
  -- | Whether to add a space after the completion. Defaults to 'True'.
  --
  -- Set this value to 'False' if the completion is only a prefix of the final
  -- valid values.
  cioAddSpace :: Bool,

  -- | Whether to treat the completions as file names (if they exists) and
  -- add a trailing slash to completions that are directories.
  -- Defaults to 'True'
  cioFiles :: Bool
}

The smart constructor mkCompleter remains unchanged, which is great for backwards compatibility. The Types module does expose the Completer(Completer) constructor, but I suppose most packages use the Options.Applicative module.

@roberth
Copy link
Contributor Author

roberth commented Feb 23, 2021

This changes the protocol from Haskell to the shell integrations. It allows more room for new types of information to be passed to the shells; even shell-specific things that do not affect other shell because the integrations will just ignore the new %whatever foo bar lines.

I propose to add to the wiki page that explains the integration:


The list of completions is written to stdout in a format that looks like

%value
completion_no_1
%files
%value
completion_no_2
%value
completion_no_3

Each completion is preceded by a %value line, which is in turn preceded by any options that apply to the completion.
In the example, completion_no_1 and completion_no_3 are completions with minimal functionality, whereas completion_no_2 should be treated as a file path, appending a trailing slash if it exists as a directory.

New "keywords" can be added for new hints that affect one or more of the shell implementations. Those can potentially make use of the rest of the line to carry extra information about the hint.


@HuwCampbell
Copy link
Collaborator

Cheers. I haven't had a hard look at this yet, but I'm all for richer and better completions.

I would really like our output to be backwards compatible though. People often don't change their completion scripts for years, while their optparse equiped binaries might upgrade to a new version at any time.

If we're going to develop a richer interface, it should be behind a separate command or flag. There's already one of these for "richness".

@HuwCampbell
Copy link
Collaborator

I'm doing a bit of background research over here.

I've seen a few small proposals for standarisation of completion scripts, so I'm looking at whether this functionality is supported in those.

I would, in general, like to encourage more tools in more languages into doing what optparse-applicative does. Writing custom completion scripts in bash is a ridiculous exercise IMO.

@roberth
Copy link
Contributor Author

roberth commented Mar 1, 2021

I've added a version option for compatibility and I've changed the CompletionItemOption monoid. I haven't tested the updated shell integration scripts yet.

Did you find a useful set of standard completion scripts?

@HuwCampbell
Copy link
Collaborator

It took me a while to find this. It's a proposal for essentially a protocol like optparse's.

https://github.com/oilshell/oil/wiki/Shellac-Protocol-Proposal
https://github.com/oilshell/oil/wiki/Shellac-Protocol-Proposal-V2

I'll let you have a read and I will go into it further later this week. We can then discuss if it's up to task.

@roberth
Copy link
Contributor Author

roberth commented Mar 15, 2021

The shellac protocol looks ok at first, but I've found some issues.

  • First of all, its documentation is incomplete, making reasoning about it unnecessarily hard,
  • it seems to ignore anything after the cursor, which is not acceptable in my case,
  • it doesn't seem to support partial completions (addspace/nospace),
  • extensibility is questionable (as is also pointed out near the bottom of https://www.redox-os.org/news/rsoc-ion-ux-3/)

I hope the first one is the only real issue, so I'll ask about it.

@HuwCampbell
Copy link
Collaborator

* First of all, its documentation is incomplete, making reasoning about it unnecessarily hard,

Yeah I noticed that as well.

, " value_mode=true"
, " ;;"
, " %addspace)"
, " compopt +o nospace"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this name flipped? addspace means we use nospace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to avoid negations in names, so in the protocol it's addspace which is the inverse of bash's nospace. The inversion is achieved by +o which is the inverse of -o.
So I've simplified the protocol semantics slightly, at the cost of some implementation complexity in the bash integration.

, " compopt +o nospace"
, " ;;"
, " %files)"
, " compopt -o filenames"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious what your thoughts are about delegating back to the completion script for file names instead of doing the bash dance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems like a good idea for another PR. It makes #408 obsolete and removes the dependency on the ancient bash that comes with macOS.

@HuwCampbell
Copy link
Collaborator

HuwCampbell commented Mar 17, 2021

Overall I think this is looking very reasonable. It's not too complex a protocol.

We'll see if anyone gets back to us regarding shellac.

@teto
Copy link

teto commented Feb 5, 2022

adding a completionItem makes sense IMo, I did it too to ease some integration with haskeline https://github.com/teto/optparse-applicative/blob/ad5d43ad8f4cbca13ca4d0658926e81a2d1e7143/src/Options/Applicative/Types.hs#L332

@roberth
Copy link
Contributor Author

roberth commented Aug 24, 2022

Is this blocked on rebase or is there something else I need to change?

@seanparsons
Copy link

@roberth I'd certainly appreciate it, I've just been totally baffled as to why some completion was getting a trailing forward slash in a tool I built and only just stumbled on this.

@roberth roberth force-pushed the completion-control branch 2 times, most recently from 96473f0 to 2e4737f Compare October 19, 2022 12:30
@roberth
Copy link
Contributor Author

roberth commented Oct 19, 2022

I've started a draf spec for the protocol, calling it ACES: Auto-Completion for Executables and Shells. It's close to what this PR implements, but uses --aces-* instead of --bash-* to be a bit more tech neutral. Ping or comment if you're interested.
https://gist.github.com/roberth/3482fe8ef8a295ce69589c241f1330ed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants