Fixing bug where native.exe --<tab> would not call native completer #3633

powercode · 2017-04-24T00:11:12Z

A case was missing when parsing the processing the current token. The result was that the registered argument completer for a native command was not called if the wordToComplete was '--'.

iSazonov · 2017-04-24T03:27:56Z

test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

@@ -0,0 +1,72 @@
+Describe TabCompletion -Tags CI {


Please use our common template:
Describe "TabCompletion"

iSazonov · 2017-04-24T03:30:45Z

test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

+        BeforeAll {
+            $nativeCommand = (Get-Command -CommandType Application -TotalCount 1).Name
+        }
+        It 'Completes native commands with -' {         


I believe the template more readable:
It "Completes native commands with '-'" {

Below too.

iSazonov · 2017-04-24T03:33:45Z

test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

+            $nativeCommand = (Get-Command -CommandType Application -TotalCount 1).Name
+        }
+        It 'Completes native commands with -' {         
+            Register-ArgumentCompleter -Native -CommandName $nativeCommand -ScriptBlock {


If we don't clean up completers after each test we should move all Register-ArgumentCompleter in BeforeAll block.

iSazonov · 2017-04-24T03:37:06Z

test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

+                else {
+                    return 'unexpected wordtocomplete'
+                }
+


Please remove extra line.

iSazonov · 2017-04-24T03:37:33Z

test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

+            $res.CompletionMatches.CompletionText | Should be "-option"                        
+        }
+    }
+}


Please add Newline at EOF.

iSazonov · 2017-04-24T03:57:23Z

src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs

@@ -114,7 +114,7 @@ internal CompletionContext CreateCompletionContext(ExecutionContext executionCon
            else
            {
                var stringExpandableToken = tokenAtCursor as StringExpandableToken;
-                if (stringExpandableToken != null && stringExpandableToken.NestedTokens != null)
+                if (stringExpandableToken?.NestedTokens != null)


We could use C# 7.0 pattern here too. It is better use common pattern in whole file.

iSazonov · 2017-04-24T04:01:27Z

src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs

@@ -156,7 +156,7 @@ private static bool CompleteAgainstSwitchFile(Ast lastAst, Token tokenBeforeCurs
            Tuple<Token, Ast> fileConditionTuple;

            var errorStatement = lastAst as ErrorStatementAst;
-            if (errorStatement != null && errorStatement.Flags != null && errorStatement.Kind != null && tokenBeforeCursor != null &&
+            if (errorStatement?.Flags != null && errorStatement.Kind != null && tokenBeforeCursor != null &&


The same about c# 7.0 pattern.

iSazonov · 2017-04-24T04:02:16Z

src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs

@@ -173,7 +173,7 @@ private static bool CompleteAgainstSwitchFile(Ast lastAst, Token tokenBeforeCurs
                }

                errorStatement = pipeline.Parent as ErrorStatementAst;
-                if (errorStatement == null || errorStatement.Kind == null || errorStatement.Flags == null)
+                if (errorStatement?.Kind == null || errorStatement.Flags == null)


The same about c# 7.0 pattern.

iSazonov · 2017-04-24T04:02:25Z

src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs

@@ -206,15 +206,13 @@ private static bool CompleteAgainstStatementFlags(Ast scriptAst, Ast lastAst, To

            // Handle "switch -f<tab>"
            var errorStatement = lastAst as ErrorStatementAst;
-            if (errorStatement != null && errorStatement.Kind != null)
+            if (errorStatement?.Kind != null)


The same about c# 7.0 pattern.

iSazonov · 2017-04-24T04:05:07Z

src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs

@@ -882,7 +871,7 @@ internal static TypeName FindTypeNameToComplete(ITypeName type, IScriptPosition
            var arrayTypeName = type as ArrayTypeName;
            if (arrayTypeName != null)


The same about c# 7.0 pattern.

iSazonov · 2017-04-24T04:20:47Z

@powercode Please add description in the PR. This can be useful for documentation.

powercode · 2017-04-24T06:21:30Z

@iSazonov Thanks for the review.
I've addressed your comments, except for the one about the registration of TabCompleters.
These are stored in private variables in the ExecutionContext, and as far as I know, there is no way to Unregister-ArgumentCompleter. @lzybkr ?

iSazonov · 2017-04-24T06:55:04Z

I think it's even better for tests to register several TabCompleters in BeforeAll and not remove them.

iSazonov · 2017-04-24T06:56:48Z

src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs

-                var stringExpandableToken = tokenAtCursor as StringExpandableToken;
-                if (stringExpandableToken != null && stringExpandableToken.NestedTokens != null)
+                if (tokenAtCursor  is  StringExpandableToken stringExpandableToken &&
+                    stringExpandableToken.NestedTokens != null)


Looks gorgeous! 👍

I'd like to see the same everywhere below. Now there are too many use cases for "as" and "?"

iSazonov · 2017-04-24T06:59:42Z

src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs

            var errorStatement = lastAst as ErrorStatementAst;
-            if (errorStatement != null && errorStatement.Flags != null && errorStatement.Kind != null && tokenBeforeCursor != null &&
-                errorStatement.Kind.Kind.Equals(TokenKind.Switch) && errorStatement.Flags.TryGetValue("file", out fileConditionTuple))
+            if (errorStatement?.Flags != null && errorStatement.Kind != null && tokenBeforeCursor != null &&


For information only. Personally I do not like to use "?" in if(). Expanded condition looks much clearer.

iSazonov · 2017-04-24T07:05:08Z

test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

+                else {
+                    return "unexpected wordtocomplete"
+                }
+


Please remove extra line.

iSazonov · 2017-04-24T07:05:27Z

test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

+                else {
+                    return "unexpected wordtocomplete"
+                }
+


Please remove extra line.

from tokenAtCursor.Text.

coaleasing null checks and type patterns.

Addressing review comments by iSazonov

powercode · 2017-04-25T06:40:59Z

Rebased master and fixed conflicting test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

lzybkr

Would it be unreasonable to ask for logic code changes only?

The C# 7 language usage is nice, but I'd rather see those sorts of changes in a distinct PR.

iSazonov · 2017-05-23T07:19:44Z

Sometimes mantainers aren't predictable - I just asked what I was getting myself.

10000

lzybkr · 2017-05-23T20:34:52Z

Sometimes people aren't predictable.

PowerShell is no longer my full time job - so when I ask for something like this, it's to make things easier for me. It's already somewhat hard to keep up as a maintainer with an unrelated full time job and non-computer related responsibilities and hobbies.

If you could see our pre-GitHub history, you would certainly see that I've been guilty of changes like this, but I blame not having a good enough VCS to make it easier. So I have plenty of personal experience with the difficulties of looking at changes that mix style and logical changes - and I do think a cleaner and easier to understand history is valuable in the long run, even if it wasn't painful to review something like this in the short term.

iSazonov · 2017-05-24T04:51:14Z

I meant that often we contribute and discuss before the appointment of mantainer and before his posts - so we can not predict a review style and facilitate the mantainer's work.
I believe our workflow process could be improved, I guess we could take more from the CoreCLR/CoreFX workflows.
/cc @joeyaiello

lzybkr · 2017-05-24T17:13:04Z

Note that CoreFX says this:

DO NOT send PRs for upgrading code to use newer language features, though it's ok to use newer language features as part of new code that's written.

See here.

iSazonov · 2017-05-24T17:36:04Z

Yes, they were more clearly writing the workflow. We have to do the same.

lzybkr · 2017-06-07T23:29:21Z

lzybkr · 2017-06-07T23:33:50Z

test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

+
+    Context NativeCommand {
+        BeforeAll {
+            $nativeCommand = (Get-Command -CommandType Application -TotalCount 1).Name


It might be wiser to create an external command on testdrive and change the path, but that's a tiny bit annoying to make it portable, so I'll merge as is.

msftclas added the cla-already-signed label Apr 24, 2017

iSazonov reviewed Apr 24, 2017

View reviewed changes

daxian-dbw assigned lzybkr Apr 24, 2017

Staffan Gustafsson added 6 commits April 25, 2017 08:30

Adding failing test

bc39584

Extract variable tokenAtCursorText

361cf7c

from tokenAtCursor.Text.

Adding support for double-dash native completion

35cb4a1

Cleanup

4fbaaa7

coaleasing null checks and type patterns.

Fixing styling issues

3024cd6

Addressing review comments by iSazonov

manual merge of TabCompletion.Tests.ps1

2727ca9

powercode force-pushed the native-arg-hyphen branch from d88a7d5 to 2727ca9 Compare April 25, 2017 06:39

Merge branch 'master' into native-arg-hyphen

0d7888e

lzybkr reviewed May 22, 2017

View reviewed changes

Removing usage of C#7 constructs

cfcfffa

lzybkr approved these changes Jun 7, 2017

View reviewed changes

lzybkr merged commit 3125764 into PowerShell:master Jun 7, 2017

powercode deleted the native-arg-hyphen branch January 26, 2018 19:05

OpportunityLiu mentioned this pull request Jan 25, 2020

improve tab complete xmake-io/xmake#673

Merged

		@@ -882,7 +871,7 @@ internal static TypeName FindTypeNameToComplete(ITypeName type, IScriptPosition
		var arrayTypeName = type as ArrayTypeName;
		if (arrayTypeName != null)

Fixing bug where native.exe --<tab> would not call native completer #3633

Fixing bug where native.exe --<tab> would not call native completer #3633

Uh oh!

Conversation

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!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!