Autocompletion #98

mauricel · 2021-03-31T18:14:21Z

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 · 2021-04-01T07:50:56Z

Take your time ;)

RichiCoder1 · 2021-04-10T20:48:08Z

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 · 2021-04-11T15:52:22Z

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 · 2021-04-12T04:53:51Z

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

Tyrrrz · 2021-04-12T11:48:28Z

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 · 2021-04-12T14:34:31Z

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.

… not handled consistently.

…x subsystem for Windows behave weirdly when application is in a windows mount (eg /mnt/c...).

…ot exist.

…with [suggest] --install command line.

…s when searching for an exact options match.

…encies: actions/runner-images#3038

mauricel · 2021-04-13T19:53:30Z

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 · 2021-04-15T12:29:41Z

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 · 2021-04-19T18:04:38Z

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 · 2021-04-29T19:07:18Z

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

Thanks! Finally got around to read through this. Left some comments/questions, which should be enough for first pass. Let's slowly flesh it out :)

Tyrrrz · 2021-05-04T13:53:17Z

CliFx.Tests/SuggestDirectiveSpecs.cs

+        private string _cmdCommandCs = @"
+[Command(""cmd"")]
+public class Command : ICommand
+{
+    public ValueTask ExecuteAsync(IConsole console) => default;
+}
+";
+
+        private string _cmd2CommandCs = @"
+[Command(""cmd02"")]
+public class Command02 : ICommand
+{
+    public ValueTask ExecuteAsync(IConsole console) => default;
+}
+";
+
+        private string _parentCommandCs = @"
+[Command(""parent"")]
+public class ParentCommand : ICommand
+{
+    public ValueTask ExecuteAsync(IConsole console) => default;
+}
+";
+
+        private string _childCommandCs = @"
+[Command(""parent list"")]
+public class ParentCommand : ICommand
+{
+    public ValueTask ExecuteAsync(IConsole console) => default;
+}
+";


Let's keep the commands local to the tests. This was the intention behind compiling them like this, otherwise we'd just be able to use regular classes instead :)

Code duplication is fine, logical isolation is more important. I don't want the tests to have shared context.

To compile multiple commands at once, you can use DynamicCommandBuilder.CompileMany(...).

Tyrrrz · 2021-05-04T13:58:05Z

CliFx.Tests/SuggestDirectiveSpecs.cs

+        }
+
+        [Fact]
+        public async Task Suggest_directive_is_disabled_by_default()


Is "off by default" a behavior we want? I mean, the user will still have to enable suggest mode in their terminal regardless, right?

Tyrrrz · 2021-05-04T13:58:45Z

CliFx.Tests/SuggestDirectiveSpecs.cs

+        [InlineData("supply all commands if nothing supplied",
+                        "clifx.exe", 0, new[] { "cmd", "cmd02", "parent", "parent list" })]
+        [InlineData("supply all commands that 'start with' argument",
+                        "clifx.exe c", 0, new[] { "cmd", "cmd02" })]
+        [InlineData("supply command options if match found, regardles of other partial matches (no options defined)",
+                        "clifx.exe cmd", 0, new string[] { })]
+        [InlineData("supply nothing if no commands 'starts with' argument",
+                        "clifx.exe m", 0, new string[] { })]
+        [InlineData("supply completions of partial child commands",
+                        "clifx.exe parent l", 0, new[] { "list" })]
+        [InlineData("supply all commands that 'start with' argument, allowing for cursor position",
+                        "clifx.exe cmd", -2, new[] { "cmd", "cmd02" })]
+        public async Task Suggest_directive_suggests_commands_by_environment_variables(string usecase, string variableContents, int cursorOffset, string[] expected)


Let's split this into separate tests instead of using InlineData. This is very unreadable in the current state in my opinion.

Tyrrrz · 2021-05-04T14:00:36Z

CliFx.Tests/SuggestDirectiveSpecs.cs

+
+            // Act
+            var exitCode = await application.RunAsync(
+                new[] { "[suggest]", "--envvar", "CLIFX-{GUID}", "--cursor", (variableContents.Length + cursorOffset).ToString() },


Do we need a dynamic environment variable or can it always be the same? It's set per-process so there shouldn't be conflicts, right?

Tyrrrz · 2021-05-04T14:03:29Z

Readme.md

+
+Once enabled, your shell must be configured to use suggest mode as follows:
+
+1. Add your application to the PATH


This sounds like a dealbreaker. Can we somehow work around this? As a user, I wouldn't want to add a CLI to PATH just to enable autocompletion...

Tyrrrz · 2021-05-04T14:25:08Z

CliFx/Suggestions/Design.md

+CliFx.Demo.exe [suggest] CliFx.Demo book add title_of_book
+# completion suggestions provided, one per line, in standard output
+```
+* The command line, and the cursor position can also be supplied by environment variable for more accurate results.


I remember you said that the issue was that PowerShell had "breaking weirdness". Can you give some examples of when/how it happens (to explain why we use environment variable as a workaround)?

Tyrrrz · 2021-05-04T14:30:09Z

CliFx/Suggestions/Design.md

+* Windows Powershell, detected by the presence of "C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe"
+* Ubuntu Powershell, detected by the presence of "/usr/bin/pwsh"
+* Ubuntu Bash, detected by the presence of "/usr/bin/bash"


If I have Ubuntu and both Powershell and Bash installed, what happens if I run cli [suggest] --install? Will it install on both or just Powershell?

Also, what if I'm using an entirely different terminal? I guess the command will execute successfully, but then autocomplete won't work, which may confuse the user.

Tyrrrz · 2021-05-04T14:33:10Z

CliFx/Suggestions/Design.md

+
+Observations:
+1. Text passed to the `[suggest]` directive is split inconsistently when passed via the powershell hook.
+2. Cursor positioning seems difficult to predict in relation to the rest of the supplied terms. Note that CliFx is supplied with command line arguments after processing for whitespace. I couldn't find a platform indepdenant way of extracting the raw command line string. 


I know on .NET/Powershell Environment.CommandLine returns the raw string without processing.

Tyrrrz · 2021-05-04T14:35:02Z

CliFx/Suggestions/Design.md

+
+# Known Issues
+
+### Bash auto-completion does not suggest child commands in empty directories


In empty directories?

Tyrrrz · 2021-05-04T14:36:31Z

NuGet.Config

+<?xml version="1.0" encoding="utf-8"?>
+<configuration>
+  <packageSources>
+    <add key="nuget.org" value="https://api.nuget.org/v3/index.json" protocolVersion="3" />
+  </packageSources>
+</configuration>


Can you merge origin/master into your branch? We added NuGet.config recently there.

Tyrrrz · 2021-06-21T14:22:30Z

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 changed the title ~~#13 suggest mode~~ Autocompletion Apr 1, 2021

Tyrrrz added the enhancement New feature or request label Apr 1, 2021

mauricel force-pushed the #13-suggest-mode branch from bd3aa95 to 5b0eedb Compare April 12, 2021 15:06

mauricel added 20 commits April 12, 2021 17:56

Add CliApplicationBuilder.AllowSuggestMode() support.

d9e489e

CommandLineSplitter utility, used to split input from powershell

2f297be

Implement command suggestions

b411301

Add tests - suggestions by command line only

1dbd0ad

Add tests - suggest takes cursor positioning into account. Tidy up.

4e256e7

Change from 'contain' matching to 'starts with' matching.

127c8e4

Add installation code for suggest mode.

2d854bd

Fix tests - disable suggest mode by default.

db3b956

Fix assertions that rely on new lines.

8d77dac

Remove compile warnings

96b0a01

Fix potential suggest install issue -- whitespace in command name not…

170b0a3

… not handled consistently.

Attempt to fix issues with OS detection.

319ea9e

Ignore false issues around OS detection. Applications running in Linu…

19733ff

…x subsystem for Windows behave weirdly when application is in a windows mount (eg /mnt/c...).

Refactor and fix directory creation issues when directory tree does n…

02fc91e

…ot exist.

Fix line ending issue causing syntax errors in unix shell hooks.

6973912

Fix bash autocomplete hook.

cffd1c8

Add suggest hook installation tests.

a5a3e6e

Remove auto-install of suggest hooks. Hooks can be installed by user …

cc47288

…with [suggest] --install command line.

Ensure any backups created during installation don't get auto-deleted.

a7f467f

Normalise line endings

7f0fe4d

mauricel force-pushed the #13-suggest-mode branch from f1e321c to 7f0fe4d Compare April 12, 2021 16:56

mauricel added 10 commits April 12, 2021 22:48

Implement options autosuggestion.

e67a9c2

Fix bug: suggest now knows the difference between ShortNames and Name…

09108a3

…s when searching for an exact options match.

Implement parameter suggestions.

8cd92d6

Update readme with usage documentation for [suggest] mode.

0af03e0

Don't provide suggestions when installing.

a01564e

Workaround for github action issue -- unable to retrieve nuget depend…

49997e4

…encies: actions/runner-images#3038

Fix bug: child command suggestions are now supplied.

2ea950d

Fix bug: bash option auto-completion.

7750827

Update [suggest] scripts in Readme.md

265ec19

Fix bug: ensure space characters do not delimit autocompletions in bash.

357307d

Add design documentation.

d615e0c

Tyrrrz reviewed May 4, 2021

View reviewed changes

Tyrrrz added invalid This doesn't seem right and removed enhancement New feature or request labels Jun 21, 2021

Tyrrrz closed this Jun 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autocompletion #98

Autocompletion #98

mauricel commented Mar 31, 2021 •

edited by Tyrrrz

Tyrrrz commented Apr 1, 2021

RichiCoder1 commented Apr 10, 2021 •

edited

mauricel commented Apr 11, 2021

RichiCoder1 commented Apr 12, 2021

Tyrrrz commented Apr 12, 2021

mauricel commented Apr 12, 2021

mauricel commented Apr 13, 2021

Tyrrrz commented Apr 15, 2021

mauricel commented Apr 19, 2021

RichiCoder1 commented Apr 29, 2021

Tyrrrz left a comment

Tyrrrz May 4, 2021

Tyrrrz May 4, 2021

Tyrrrz May 4, 2021

Tyrrrz May 4, 2021

Tyrrrz May 4, 2021

Tyrrrz May 4, 2021

Tyrrrz May 4, 2021

Tyrrrz May 4, 2021

Tyrrrz May 4, 2021

Tyrrrz May 4, 2021

Tyrrrz commented Jun 21, 2021


		Once enabled, your shell must be configured to use suggest mode as follows:

		1. Add your application to the PATH


		# Known Issues

		### Bash auto-completion does not suggest child commands in empty directories

Autocompletion #98

Autocompletion #98

Conversation

mauricel commented Mar 31, 2021 • edited by Tyrrrz

Tyrrrz commented Apr 1, 2021

RichiCoder1 commented Apr 10, 2021 • edited

mauricel commented Apr 11, 2021

RichiCoder1 commented Apr 12, 2021

Tyrrrz commented Apr 12, 2021

mauricel commented Apr 12, 2021

mauricel commented Apr 13, 2021

Tyrrrz commented Apr 15, 2021

mauricel commented Apr 19, 2021

RichiCoder1 commented Apr 29, 2021

Tyrrrz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tyrrrz commented Jun 21, 2021

mauricel commented Mar 31, 2021 •

edited by Tyrrrz

RichiCoder1 commented Apr 10, 2021 •

edited