Add Microsoft.PowerShell.UnixCompleters module by rjmholt · Pull Request #44 · PowerShell/Modules

rjmholt · 2020-02-14T22:59:48Z

No description provided.

rjmholt · 2020-02-20T17:42:22Z

Modules/Microsoft.PowerShell.UnixCompleters/Microsoft.PowerShell.UnixCompleters.psd1

iSazonov · 2020-02-21T03:22:23Z

Modules/Microsoft.PowerShell.UnixCompleters/Microsoft.PowerShell.UnixCompleters.psd1

+CompanyName = 'Microsoft Corporation'
+
+# Copyright statement for this module
+Copyright = '© Microsoft Corporation'


Should we replace the Unicode with "(C)"?

Could do if that's deemed to be more appropriate

I believe convention is to stick with ASCII where possible, so use (c) (lowercase c)

iSazonov · 2020-02-21T03:22:55Z

Modules/Microsoft.PowerShell.UnixCompleters/Microsoft.PowerShell.UnixCompleters.psd1

+Description = 'Get parameter completion for native Unix utilities. Requires zsh or bash.'
+
+# Minimum version of the PowerShell engine required by this module
+PowerShellVersion = '6.0'


Was this tested on 6.0?

No, but I saw no reason to limit it. There's no constraint I can think of (other than possibly the .NET version) that would stop it from working on downlevel PowerShell versions. Certainly I tested it with 6.2 and it worked fine.

Why I don't see GitHub "Resolved" button?

Why I don't see GitHub "Resolved" button?

Oh interesting! Perhaps it's linked to being a "member" or "contributor" of some other privileged role?

iSazonov · 2020-02-21T03:24:05Z

Modules/Microsoft.PowerShell.UnixCompleters/Microsoft.PowerShell.UnixCompleters.psd1

+PowerShellVersion = '6.0'
+
+# Name of the PowerShell host required by this module
+# PowerShellHostName = ''


Please remove commented and unused lines.

iSazonov · 2020-02-21T03:24:58Z

Modules/Microsoft.PowerShell.UnixCompleters/Microsoft.PowerShell.UnixCompleters.psd1

+# RequiredAssemblies = @()
+
+# Script files (.ps1) that are run in the caller's environment prior to importing this module.
+ScriptsToProcess = @("OnStart.ps1")


I think the name should be more specific and correlate with the module name.

Well it's already encapsulated by the module. We can't call every script in a module "Microsoft.PowerShell.UnixCompleters.OnStart.ps1". What would you call it?

I'd be ok with short name until users do not see them in logs, traces and so on. Otherwise full module name prefix is good.

If we had to name every script in a module like that it would get ridiculous. The name of the script is contextual to the module

.../Microsoft.PowerShell.UnixCompleters/Microsoft.PowerShell.UnixCompleters/ZshUtilCompleter.cs

iSazonov · 2020-02-21T04:09:29Z

.../Microsoft.PowerShell.UnixCompleters/Microsoft.PowerShell.UnixCompleters/ZshUtilCompleter.cs

+            using (var zshProc = new Process())
+            {
+                zshProc.StartInfo.RedirectStandardOutput = true;
+                zshProc.StartInfo.RedirectStandardError = true;
+                zshProc.StartInfo.FileName = this._zshPath;
+                zshProc.StartInfo.Arguments = arguments;
+
+                zshProc.Start();
+
+                return zshProc.StandardOutput.ReadToEnd();
+            }


The same about using and Process() initialization.

iSazonov · 2020-02-21T04:09:48Z

.../Microsoft.PowerShell.UnixCompleters/Microsoft.PowerShell.UnixCompleters/ZshUtilCompleter.cs

+                .ToString();
+        }
+    }
+}


Please add new line.

Modules/Microsoft.PowerShell.UnixCompleters/OnStart.ps1

Modules/Microsoft.PowerShell.UnixCompleters/tests/Completion.Tests.ps1

…ll.UnixCompleters/BashUtilCompleter.cs Co-Authored-By: Ilya <darpa@yandex.ru>

rjmholt · 2020-02-21T07:40:12Z

@iSazonov while I'm grateful for the review, this is really just me checking in code from https://github.com/rjmholt/PSUnixUtilCompleters, which has already been released. I'm probably not going to make any major edits in this PR, since it's just a migration.

rjmholt · 2020-03-19T16:33:58Z

@iSazonov I've addressed the easier parts of your feedback. I've left the constants and the string localisation

rjmholt · 2020-03-19T16:34:09Z

@SteveL-MSFT this should be ready to merge now

...Microsoft.PowerShell.UnixCompleters/Microsoft.PowerShell.UnixCompleters/BashUtilCompleter.cs

...l.UnixCompleters/Microsoft.PowerShell.UnixCompleters/Commands/ImportUnixCompletersCommand.cs

...l.UnixCompleters/Microsoft.PowerShell.UnixCompleters/Commands/RemoveUnixCompletersCommand.cs

...icrosoft.PowerShell.UnixCompleters/Microsoft.PowerShell.UnixCompleters/UnixUtilCompletion.cs

Modules/Microsoft.PowerShell.UnixCompleters/OnStart.ps1

TravisEz13

My issues are resolved

daxian-dbw · 2020-04-13T18:38:55Z

Modules/Microsoft.PowerShell.UnixCompleters/OnStart.ps1

+$null = Register-EngineEvent -SourceIdentifier PowerShell.OnIdle -MaxTriggerCount 1 -Action {
+    $m = Get-Module PSUnixUtilCompleters
+    $m.OnRemove = {
+        Write-Verbose "Deregistering UNIX native util completers"


This is better moved to a psm1 file, where you can use $MyInvocation.MyCommand.Module.OnRemove.

After actually trying this out, it doesn't seem to work: $MyInvocation.MyCommand.Module is null, I suspect because the psm1 being run is still in the dot-source phase; it hasn't been created as a module yet. That's why I had to be tricky with that event

Try $MyInvocation.MyCommand.ScriptBlock.Module.OnRemove instead. This one should work.

daxian-dbw · 2020-04-13T18:45:19Z

...ft.PowerShell.UnixCompleters/Microsoft.PowerShell.UnixCompleters/UtilCompleterInitializer.cs

+            CompleterGlobals.CompletedCommands = utilsToComplete;
+            CompleterGlobals.UnixUtilCompleter = utilCompleter;
+
+            RegisterCompletersForCommands(utilsToComplete);


Will there be a problem if the module gets imported to two Runspaces? namely, Register-ArgumentCompleter would be called twice for each of the same native utility.

Quite likely. I don't think it's threadsafe either (nor is the underlying mechanism for Register-ArgumentCompleter; it's not a ConcurrentDictionary<,>). But as a module intended solely for interactive scenarios, I'm thinking of multithreaded/multirunspace scenarios as out of scope for now.

That's a fair point.

rjmholt added 2 commits February 14, 2020 14:59

Add UnixCompleters module

fa95168

Remove module licsense

b95c0ec

rjmholt added 2 commits February 20, 2020 09:47

Rename commands

3d127a3

Fix naming

91bbea9

iSazonov reviewed Feb 21, 2020

View reviewed changes

Update Modules/Microsoft.PowerShell.UnixCompleters/Microsoft.PowerShe…

22e0e44

…ll.UnixCompleters/BashUtilCompleter.cs Co-Authored-By: Ilya <darpa@yandex.ru>

iSazonov mentioned this pull request Feb 22, 2020

Add standard docs in the repo #52

Closed

Address @iSazonov's feedback

a787d10

rjmholt mentioned this pull request Apr 13, 2020

Ditch the license acceptance rjmholt/PSUnixUtilCompleters#9

Closed

rjmholt added 2 commits April 13, 2020 11:24

Update copyright symbol

d028662

Add PSGallery tags

137fce5

TravisEz13 suggested changes Apr 13, 2020

View reviewed changes

Address @TravizEz13's feedback

4f7a138

TravisEz13 approved these changes Apr 13, 2020

View reviewed changes

Add missed newline

231b06e

daxian-dbw reviewed Apr 13, 2020

View reviewed changes

rjmholt and others added 3 commits April 13, 2020 11:48

Convert to psm1

e3a18b7

Fix build script

7668b0c

Move back to registration hook in script

9f78c36

TravisEz13 merged commit a82fd29 into PowerShell:master Apr 13, 2020

Conversation

rjmholt commented Feb 14, 2020

Uh oh!

rjmholt commented Feb 20, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rjmholt commented Feb 21, 2020

Uh oh!

rjmholt commented Mar 19, 2020

Uh oh!

rjmholt commented Mar 19, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TravisEz13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rjmholt Apr 13, 2020 •

edited

Loading