Fixing bug where native.exe --<tab> would not call native completer by powercode · Pull Request #3633 · PowerShell/PowerShell · GitHub

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 '--'.

Please use our common template:
Describe "TabCompletion"

Closed.

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

Below too.

Closed.

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

Please remove extra line.

Closed.

Please add Newline at EOF.

Closed.

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

The same about c# 7.0 pattern.

Closed.

Staffan Gustafsson (@powercode) Please add description in the PR. This can be useful for documentation.

Ilya (@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. Jason Shirk (@lzybkr) ?

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

Looks gorgeous! 👍

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

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

Please remove extra line.

Closed.

Please remove extra line.

Closed.

from tokenAtCursor.Text.

coaleasing null checks and type patterns.

Addressing review comments by iSazonov

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

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.

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

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.

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 Joey Aiello (@joeyaiello)

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.

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

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.

…erShell#3633)

Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Apr 24, 2017

Ilya (iSazonov) reviewed Apr 24, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) assigned Jason Shirk (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

Staffan Gustafsson (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

Jason Shirk (lzybkr) reviewed May 22, 2017

View reviewed changes

Removing usage of C#7 constructs

cfcfffa

Jason Shirk (lzybkr) approved these changes Jun 7, 2017

View reviewed changes

Jason Shirk (lzybkr) merged commit 3125764 into PowerShell:master Jun 7, 2017

Staffan Gustafsson (powercode) deleted the native-arg-hyphen branch January 26, 2018 19:05

Opportunity (OpportunityLiu) mentioned this pull request Jan 25, 2020

improve tab complete xmake-io/xmake#673

Merged

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026

Fix bug where native.exe --<tab> would not call native completer (Pow…

81f8329

…erShell#3633)

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

Fixing bug where native.exe --<tab> would not call native completer by powercode · Pull Request #3633 · PowerShell/PowerShell · GitHub

Uh oh!

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

Fixing bug where native.exe --<tab> would not call native completer#3633Jason Shirk (lzybkr) merged 8 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom powercode:native-arg-hyphenpowercode/PowerShell:native-arg-hyphenCopy head branch name to clipboard

Conversation

Staffan Gustafsson (powercode) commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Ilya (iSazonov) commented Apr 24, 2017

Uh oh!

Staffan Gustafsson (powercode) commented Apr 24, 2017

Uh oh!

Ilya (iSazonov) commented Apr 24, 2017

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!

Staffan Gustafsson (powercode) commented Apr 25, 2017

Uh oh!

Jason Shirk (lzybkr) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented May 23, 2017

Uh oh!

Jason Shirk (lzybkr) commented May 23, 2017

Uh oh!

Ilya (iSazonov) commented May 24, 2017

Uh oh!

Jason Shirk (lzybkr) commented May 24, 2017

Uh oh!

Ilya (iSazonov) commented May 24, 2017

Uh oh!

Jason Shirk (lzybkr) commented Jun 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Fixing bug where native.exe --<tab> would not call native completer#3633
Jason Shirk (lzybkr) merged 8 commits into
PowerShell:masterfrom
powercode:native-arg-hyphen

Staffan Gustafsson (powercode) commented Apr 24, 2017 •
edited
Loading