Autocompletion - Githubissues

mauricel commented 3 years ago

Hi,

I've been working at this a while, so I thought I should at least get what I have so far for feedback.

This pull request:

Implements [suggest] directive for commands .
Implements Bash and Powershell installation upon first run for current user.
Adds necessary tests

This pull request does not:

Implement [suggest] auto-completion for options/parameters. Still to do.

Sorry for the wait; encountered some false bugs that slowed down development, and life got busy also.

Closes #13

Tyrrrz commented 3 years ago

Take your time ;)

RichiCoder1 commented 3 years ago

This is looking awesome! Did have one piece of feedback though:

Implements Bash and Powershell installation upon first run for current user.

Please don't. As a user, the only time I expect my profiles to be touched is either if A) I'm personally updating them or B) I'm using a package manager which has a trigger to enable completion. I would never want a tool to abitrarily touch my profiles. Providing the ability for tool makers to opt-in to having a command which will allow their end users to trigger an install or export a registration script would be much more preferable.

mauricel commented 3 years ago

This is looking awesome! Did have one piece of feedback though:

Implements Bash and Powershell installation upon first run for current user.

Please don't. As a user, the only time I expect my profiles to be touched is either if A) I'm personally updating them or B) I'm using a package manager which has a trigger to enable completion. I would never want a tool to abitrarily touch my profiles. Providing the ability for tool makers to opt-in to having a command which will allow their end users to trigger an install or export a registration script would be much more preferable.

I had wondered what kind of reaction this would get. My thought was to let the application developer decide as follows:

new CliApplicationBuilder().AllowSuggestMode(enabled: true, autoinstall: true)

if enabled = false, don't install under any circumstances if enabled = true, autoinstall = false, install is manually triggered by user as follows:

cli.exe [suggest] --install 
## alternatively, supply reference documentation

if enabled = true, autoinstall = true, install upon first run

The default values would be enabled = false, autoinstall = false.

RichiCoder1 commented 3 years ago

Imho, allowing auto install at all I think is a foot gun and maintenance concern. I may honestly be a bit biased however.

Tyrrrz commented 3 years ago

I'm also leaning towards letting the user decide whether they want to install autocompletion or not. We can add a hint inside the help text what would give instructions on how to do it (probably just running cli.exe [suggest] or something). Then that command would install autosuggestions for the current shell.

mauricel commented 3 years ago

Imho, allowing auto install at all I think is a foot gun and maintenance concern.

For libraries, I tend to err on giving developers ultimate control over their applications, however upon second thought, I think there is wisdom here. While I've tried my best to consider the edge cases for now, and while I reckon we can probably get there eventually, I don't for a second think we wouldn't be fixing bugs.

Ok. Let's make it a user choice, at least until we've validated functionality with real users. Even then, I suspect that we will have done more than enough with any sort of install.

I'm also leaning towards letting the user decide whether they want to install autocompletion or not.

Yup. I'm cool with that.

We can add a hint inside the help text what would give instructions on how to do it

I like this

(probably just running cli.exe [suggest] or something).

Yup. We just want to watch out for the edge case where someone wants to manually write their own hooks. We wouldn't want the action that does suggestions to also modify profiles.

mauricel commented 3 years ago

Updated PR as follows

implemented parameter and option suggestions
added documentation for [suggest] mode in Readme.md
Added Nuget.config to work around recent CI failures
Number of bug fixes

Known issue: Bash autocompletion does not suggest child commands

Given:  a Bash shell, working in an empty directory
When: user types CliFx.Demo <tab><tab>
Then: Suggestions are: [book] instead of [book, book add, book list, book remove]
Note: cannot reproduce issue if 'book list' is renamed to 'boom'

I suspect the issue is in the bash autocomplete hook, but I don't know what the issue is exactly. More investigation needed.

Tyrrrz commented 3 years ago

Thanks @mauricel. I will take a closer look over the coming weeks.

Can you please write down a short but detailed reasoning behind the changes you made? I want to better understand how it works, possible edge cases, nuances you encountered and considered. Unfortunately it's hard to do by just reading the code (as it usually is, not your fault).

If you want, you can leave review comments on your code to highlight certain parts or start a discussion if you need some input. Thanks!

mauricel commented 3 years ago

Thank you for considering this pull request. I've included the requested design information. You're right about the edge cases. I hope this helps., and I would appreciate any feedback/guidance you might have.

Sorry about the large merge! Thanks!

RichiCoder1 commented 3 years ago

Random aside: Probably too big for this PR (which is already big (and awesome)), but it'd be nice to eventually be able to plug into autocompletion to allow dynamic suggestions. (e.g. git checkout f<tab> displaying branch suggestions)

Tyrrrz commented 3 years ago

Gonna close it for now due to inactivity, but feel free to reopen if you ever feel like working on it again (and it's fine if you don't too) 🙂

Tyrrrz / CliFx

Autocompletion #98