Fixing bug where native.exe --<tab> would not call native completer#3633
Fixing bug where native.exe --<tab> would not call native completer#3633lzybkr merged 8 commits intoPowerShell:masterfrom
Conversation
There was a problem hiding this comment.
Please use our common template:
Describe "TabCompletion"
There was a problem hiding this comment.
I believe the template more readable:
It "Completes native commands with '-'" {
Below too.
There was a problem hiding this comment.
If we don't clean up completers after each test we should move all Register-ArgumentCompleter in BeforeAll block.
There was a problem hiding this comment.
Please remove extra line.
There was a problem hiding this comment.
Please add Newline at EOF.
There was a problem hiding this comment.
We could use C# 7.0 pattern here too. It is better use common pattern in whole file.
There was a problem hiding this comment.
The same about c# 7.0 pattern.
There was a problem hiding this comment.
The same about c# 7.0 pattern.
There was a problem hiding this comment.
The same about c# 7.0 pattern.
There was a problem hiding this comment.
The same about c# 7.0 pattern.
|
@powercode Please add description in the PR. This can be useful for documentation. |
|
I think it's even better for tests to register several TabCompleters in BeforeAll and not remove them. |
There was a problem hiding this comment.
Looks gorgeous! 👍
I'd like to see the same everywhere below. Now there are too many use cases for "as" and "?"
There was a problem hiding this comment.
For information only. Personally I do not like to use "?" in if(). Expanded condition looks much clearer.
There was a problem hiding this comment.
Please remove extra line.
There was a problem hiding this comment.
Please remove extra line.
from tokenAtCursor.Text.
Addressing review comments by iSazonov
d88a7d5 to
2727ca9
Compare
|
Rebased master and fixed conflicting test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1 |
lzybkr
left a comment
There was a problem hiding this comment.
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. |
|
Note that CoreFX says this: See here. |
|
Yes, they were more clearly writing the workflow. We have to do the same. |
|
|
|
|
||
| Context NativeCommand { | ||
| BeforeAll { | ||
| $nativeCommand = (Get-Command -CommandType Application -TotalCount 1).Name |
There was a problem hiding this comment.
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.
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 '--'.