Enable completions for blank between comma and something

[matt9ucci]

PR Summary

Tab completions for a blank between a comma and something (e.g. parameter) does not work.

For example, the No. 1 in the following cases works well, but the No. 2 does not:

No.	o/x	Command and <tab>	lastAst	lastAst is
1	⭕	Get-Command -Type Alias,<tab>	{Alias,}	[ErrorExpressionAst]
2	❌	Get-Command -Type Alias,<tab> -All	{Alias, -All}	[ArrayLiteralAst]

That is because the tab completion is triggered if the lastAst is ErrorExpressionAst. Not triggered if ArrayLiteralAst. There are same issues other than the Get-Command, for example, dir (Get-ChildItem), Get-Alias, Get-Module, etc.

I have fixed the condition of triggering the tab completion. It is triggered if the lastAst is ErrorExpressionAst or ArrayLiteralAst.

PR Context

It helps us to modify such an array, especially when we use Alt+a key (SelectCommandArguments).

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
• User-facing changes
  • Not Applicable
• Testing - New and feature
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and opened an issue in the relevant tool repository. This may include:
    • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.

[ghost]

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

[daxian-dbw]

@MartinGC94, can you please take a quick look at this PR and see if there could be any unwanted side effects after adding || lastAst is ArrayLiteralAst in the check?

[MartinGC94]

The code doesn't work. Get-Command -Type Alias,<Tab> -All fails inside the ProcessParameter method because wordToComplete is null. The test the OP made adds a space between the comma and the cursor so it doesn't actually test the code change he made (which is why the test succeeds).

[daxian-dbw]

@MartinGC94 Thanks for catching it! Just tried and found that Get-Command -Type Alias, <Tab> -All doesn't work too as of today, so I guess the change fixed that but not -Type Alias,<tab> -All?

[MartinGC94]

No this PR doesn't seem to fix anything. Get-Command -Type Alias, <Tab> -All doesn't work because no code was changed to fix it from its current broken state. -Type Alias,<tab> -All doesn't work because of the previously mentioned null exception further down the method chain.

[matt9ucci]

@daxian-dbw @MartinGC94 Thank you for your reply and I am sorry for my mistake. I have tested it again.

It works for the following commands:

• Get-ChildItem -Path .\a.txt,<TAB> -Recurse
• Get-Alias -Name gal,<TAB> -Verbose
• Get-Module -Name PSReadLine,<TAB> -All

But it does not work, as you say, for Get-Command -Type Alias,<TAB> -All.

Why? I think it is because the -Type parameter (alias for -CommandType) accepts [System.Management.Automation.CommandTypes] enum flags. I have noticed that all the above success cases are [string[]] parameter.

Actually, my motive in the first place is to fix Trace-Command -Name CmdletProviderClasses,<TAB> -Expression {}. The -Name is also [string[]]. But I mistook the PR could solve more wide range of problems including Get-Command.

Additionally, I just care about the ,<TAB>, not the , <TAB> (comma and space). The , <TAB> completions does not work well with or without this PR. The enum flags case and the , <TAB> case might need more investigation and other PRs.

Based on the above, I would like to propose the following fixes:

• Fix test cases: replace Get-Command with some commands, for example, Get-ChildItem
• Fix PR title represent a smaller problem, for example, "Enable completions for array parameter value just after comma without space even if other parameters follow it"

[MartinGC94]

Yeah that sounds right, it did look like there was some Enum related code when I stepped through it yesterday. The only problem with a partial fix is that it causes the tab completion code to throw when you try with Enums and ideally it would never throw but throwing doesn't cause any issues as far as I know.
I would encourage you to do a full fix for the experience but if not then that's fine as well, I'll probably end up doing it if you won't 😁

[matt9ucci]

it causes the tab completion code to throw when you try with Enums and ideally it would never throw

Thank you for your advice. I have fixed it this time for sur... probably:smile:. Please review the code.

[MartinGC94]

It looks good now. If you want to also fix it with a space between the comma and cursor you can change line 4128 in CompletionCompleters from if (!arg.ParameterContainsArgument && arg.Argument.Extent.StartOffset > token.Extent.StartOffset) to:

if (token.Kind == TokenKind.Parameter
|| (token.Extent.StartOffset > arg.Argument.Extent.StartOffset && token.Extent.EndOffset < arg.Argument.Extent.EndOffset))

This also fixes the scenario where you move the cursor between a parameter with a colon and a current value like: Get-Command -CommandType: <Tab> Alias. If you decide to add this change remember to add tests for it.

[matt9ucci]

Updated @MartinGC94's suggested lines (thanks!) and test cases.

[ghost]

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

[pull-request-quantifier-deprecated]

This PR has 56 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

---

Quantification details

Label      : Small
Size       : +52 -4
Percentile : 22.4%

Total files changed: 3

Change summary by file extension:
.cs : +5 -4
.ps1 : +47 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

• Fast and predictable releases to production:
  • Optimal size changes are more likely to be reviewed faster with fewer
    iterations.
  • Similarity in low PR complexity drives similar review times.
• Review quality is likely higher as complexity is lower:
  • Bugs are more likely to be detected.
  • Code inconsistencies are more likely to be detetcted.
• Knowledge sharing is improved within the participants:
  • Small portions can be assimilated better.
• Better engineering practices are exercised:
  • Solving big problems by dividing them in well contained, smaller problems.
  • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

• Use the PullRequestQuantifier to quantify your PR accurately
  • Create a context profile for your repo using the context generator
  • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
  • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
  • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
• Change your engineering behaviors
  • For PRs that fall outside of the desired spectrum, review the details and check if:
    • Your PR could be split in smaller, self-contained PRs instead
    • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

• One line was added: +1 -0
• One line was deleted: +0 -1
• One line was modified: +1 -1 (git diff doesn't know about modified, it will
  interpret that line like one addition plus one deletion)
• Change percentiles: Change characteristics (addition, deletion, modification)
  of this PR in relation to all other PRs within the repository.

---

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

[daxian-dbw]

LGTM. Made a couple updates on the comments.

[ghost]

🎉v7.3.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes