Add single/double quote support for `Join-String` Argument Completer #25283

ArmaanMcleod · 2025-04-05T13:29:11Z

PR Summary

This PR makes sure JoinItemCompleter uses CompletionHelpers.GetMatchingResults so completions can work with single/double quotes.

There is multiple improvements with this completer which is also included in tests, e.g. completing newlines and supporting more variations/flexibility with single/double quotes. I also split JoinItemCompleter into SeparatorArgumentCompleter and FormatStringArgumentCompleter`.

I've also made the following changes in preparation to extend IArgumentCompleter further:

MatchStrategy delegates which can be implemented and overriden by user potentially. This will allow GetMatchingResults to be more flexible with how values are matched.
added new listItemTextMapping parameter to GetMatchingResults so we can customize list item text which is needed for JoinItemCompleter.

PR Context

Fixes #24873

This was the last completer I intended to move to GetMatchingResults since it requires behaviour which will help influence the interface extension in #25033.

PR Checklist

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Join-String.cs

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

…letionHelpers.cs Co-authored-by: Ilya <darpa@yandex.ru>

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Join-String.cs

iSazonov · 2025-04-09T03:41:25Z

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

                {
                    string completionText = QuoteCompletionText(value, quote);
+                    string toolTip = toolTipMapping?.Invoke(value) ?? value;
+                    string listItemText = listItemTextMapping?.Invoke(value) ?? value;


We should do the both mappings in one method. If this requires many changes in other completers it is better to implement this in follow PR.

Agree. I am actually thinking of just changing the type from Func<string, string> to Dictionary<string, (string Tooltip, string ListItemText)>. I am not sure Func is the best type to use here.

It could be Func<string, (string Tooltip, string ListItemText)>. It's not critical right now. We can postpone the decision when we choose how best to extend the interface.

Yep sounds good. Will postpone this. I'd like to just get IsMatch and ListItemTextMapping in interface in next PR.

We should do the both mappings in one method.

+ for flexibility we could consider cascading as for possible values method => static list.

ArmaanMcleod · 2025-04-09T12:02:29Z

Thanks for reviews @iSazonov. Let me know what you think with latest changes 🙂

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Join-String.cs

…n-String.cs Co-authored-by: Ilya <darpa@yandex.ru>

iSazonov · 2025-04-10T03:33:33Z

@ArmaanMcleod Please look test failures.

ArmaanMcleod · 2025-04-10T20:57:00Z

@ArmaanMcleod Please look test failures.

Thanks @iSazonov.

I think test failures are from wildcard pattern like [* not being escaped before passing to WildcardPattern.Get.

I fixed this in 09818bb

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

This reverts commit 139a778.

iSazonov · 2025-04-19T13:59:06Z

Thanks for in-depth anaysis @iSazonov.

So we should move the normalization from MatchStrategy to main method, do the normalization only if quote is single quote.

I don't think this can just apply to single quotes given newlines need to be normalized and current code forces double quotes.

Sorry, it is my typo. Right wording is "do the normalization only if quote is NOT single quote."

So ideally we should handle all, not only line endings.

We can but not sure the value in supporting all in this PR. It seems fine to do normalization with just line endings for this PR.

I mean we can add this in follow PR. I don't force you to implement this. Now I'd only move the normalization (two replace() methods) in separate method and put it in right place. This allows us easily to enhance the method in future. Obviously, it is edge case but since we want to get very user-friendly capabilities for simple completer creation the feature is "nice to have".

But it seems we haven't method to convert raw string (WordToComplete) to pwsh expandable string.
On the other hand, we already passing possible strings through the tokenizer every time in CompletionRequiresQuotes().

I think it is easier using Parser.ParseInput to get tokens convert possible values with backticks to \r\n so it can compare with word to complete. But even then this is pointless because it will still leave back ticks in which after expansion and you'll need to normalize word to complete anyways.

I considered the case but there can be some tokens and it is not easy combine them back in source string. So I concluded it is better to convert WordToComplete.

I don't think this effort is worth it if Replace() will work. I have added a NormalizeLineEndings method which runs outside of MatchStrategy delegates which works well.

We should probably keep this code as simple as possible since it can complex very fast 😄.

As I said I agree with current Replace() normalization for the PR but I suggest having it in new normalization method under right condition and in right place.

ArmaanMcleod · 2025-04-19T23:56:50Z

@iSazonov I have moved this to a method called NormalizeToExpandableString and only running this when string is double quote for normalizing to expandable string.

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

…letionHelpers.cs Co-authored-by: Ilya <darpa@yandex.ru>

test/xUnit/csharp/test_CompletionHelpers.cs

iSazonov · 2025-04-20T13:50:16Z

/azp run

azure-pipelines · 2025-04-20T13:50:27Z

Azure Pipelines successfully started running 1 pipeline(s).

iSazonov

I merge this as-is since the PR is good and too large to keep improving.

@ArmaanMcleod Thanks for your efforts!

The comments are below, which I have compiled for further work. Some of them are optional, some are important.

iSazonov · 2025-04-20T14:54:43Z

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

            IEnumerable<string> possibleCompletionValues,
            Func<string, string> toolTipMapping = null,
-            CompletionResultType resultType = CompletionResultType.Text)
+            Func<string, string> listItemTextMapping = null,


Perhaps convert these Func<string, string> to delegate.

Yep agree it should all be consolidated to delegates.

iSazonov · 2025-04-20T15:12:07Z

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

                {
                    string completionText = QuoteCompletionText(value, quote);
+                    string toolTip = toolTipMapping?.Invoke(value) ?? value;
+                    string listItemText = listItemTextMapping?.Invoke(value) ?? value;


We should do the both mappings in one method.

+ for flexibility we could consider cascading as for possible values method => static list.

iSazonov · 2025-04-20T15:15:15Z

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

+        internal static string NormalizeToExpandableString(string value)
+            => value
+                .Replace("\r", "`r")
+                .Replace("\n", "`n")
+                .Replace("\t", "`t")


The method could be optimized for performance as separate work (low priority).

This one is actually quite difficult to optimize, basic Replace approach outperforms other methods (string builder, regex etc.).

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

iSazonov · 2025-04-20T15:36:04Z

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

+        /// </remarks>
+        internal static readonly MatchStrategy WildcardPatternMatch = (value, wordToComplete)
+            => WildcardPattern
+                .Get(wordToComplete + "*", WildcardOptions.IgnoreCase)


I always wonder why we don't have GetStar(wordToComplete, WildcardOptions.IgnoreCase). At least internally.

That would be good but the name seems a bit weird. For now its fine to just append * ourselves which is everywhere in this codebase 😄.

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

iSazonov · 2025-04-20T16:02:39Z

@ArmaanMcleod Please remove my two last commits or add new commit to unlock the PR merging.

…letionHelpers.cs Co-authored-by: Ilya <darpa@yandex.ru>

ArmaanMcleod · 2025-04-21T00:51:48Z

Thanks @iSazonov.

I have added some commits from your suggestions. I think for the rest better to do that in another PR for improvements.

microsoft-github-policy-service · 2025-04-21T04:48:53Z

📣 Hey @@ArmaanMcleod, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

Microsoft Forms

…owerShell#25283)

ArmaanMcleod added 3 commits April 6, 2025 00:17

Add single/double quote support for Join-String Argument Completer

99b1ff5

Merge branch 'master' into join-string-completion-use-completion-helpers

c22bcdd

CodeFactor: SA1413 Add trailing comma in multi-line initializers

d4a5a7d

iSazonov reviewed Apr 6, 2025

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Join-String.cs Outdated Show resolved Hide resolved

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Apr 8, 2025

iSazonov reviewed Apr 8, 2025

View reviewed changes

ArmaanMcleod and others added 2 commits April 8, 2025 23:22

Split into two argument completers

401ac49

Update src/System.Management.Automation/engine/CommandCompletion/Comp…

51d9437

…letionHelpers.cs Co-authored-by: Ilya <darpa@yandex.ru>

iSazonov reviewed Apr 9, 2025

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Join-String.cs Outdated Show resolved Hide resolved

iSazonov reviewed Apr 9, 2025

View reviewed changes

ArmaanMcleod added 2 commits April 9, 2025 21:56

Fix IsMatch method and test

20ccaa7

Merge branch 'master' into join-string-completion-use-completion-helpers

ffae7ef

ArmaanMcleod requested a review from iSazonov April 9, 2025 12:02

iSazonov reviewed Apr 9, 2025

View reviewed changes

ArmaanMcleod and others added 2 commits April 10, 2025 06:37

Update src/Microsoft.PowerShell.Commands.Utility/commands/utility/Joi…

8d556b4

…n-String.cs Co-authored-by: Ilya <darpa@yandex.ru>

Fixed indentation

163a6ec

ArmaanMcleod added 2 commits April 11, 2025 06:53

Escape word to complete before it gets to WildcardPattern.Get

09818bb

Merge branch 'master' into join-string-completion-use-completion-helpers

e8fe981

Fix Xunit test

d57f8c8

iSazonov reviewed Apr 11, 2025

View reviewed changes

src/System.Management.Automation/engine/CommandCompletion/CompletionHelpers.cs Outdated Show resolved Hide resolved

ArmaanMcleod added 2 commits April 12, 2025 23:29

Add IsMatch delegate which includes escape strategy delegate

33ca0a7

Merge branch 'master' into join-string-completion-use-completion-helpers

3258b66

iSazonov reviewed Apr 12, 2025

View reviewed changes

src/System.Management.Automation/engine/CommandCompletion/CompletionHelpers.cs Outdated Show resolved Hide resolved

ArmaanMcleod added 2 commits April 13, 2025 12:46

Add MatchStrategy delegates

f159a33

Wrap delegates on multiple lines

99f2e3a

iSazonov reviewed Apr 13, 2025

View reviewed changes

src/System.Management.Automation/engine/CommandCompletion/CompletionHelpers.cs Show resolved Hide resolved

iSazonov self-assigned this Apr 13, 2025

ArmaanMcleod added 3 commits April 19, 2025 22:31

Merge branch 'master' into join-string-completion-use-completion-helpers

f4bfa8e

Check newlines in conditions before replacing

139a778

Revert "Check newlines in conditions before replacing"

bcc9af4

This reverts commit 139a778.

Rename method to NormalizeToExpandableString

e7153fa

Add more special character to escape sequence normalizing

0022a79

iSazonov reviewed Apr 20, 2025

View reviewed changes

src/System.Management.Automation/engine/CommandCompletion/CompletionHelpers.cs Outdated Show resolved Hide resolved

Update src/System.Management.Automation/engine/CommandCompletion/Comp…

89da820

…letionHelpers.cs Co-authored-by: Ilya <darpa@yandex.ru>

ArmaanMcleod requested a review from iSazonov April 20, 2025 11:08

iSazonov reviewed Apr 20, 2025

View reviewed changes

test/xUnit/csharp/test_CompletionHelpers.cs Show resolved Hide resolved

Update test/xUnit/csharp/test_CompletionHelpers.cs

7ea1680

iSazonov reviewed Apr 20, 2025

View reviewed changes

test/xUnit/csharp/test_CompletionHelpers.cs Outdated Show resolved Hide resolved

Update test/xUnit/csharp/test_CompletionHelpers.cs

6156af3

iSazonov approved these changes Apr 20, 2025

View reviewed changes

ArmaanMcleod and others added 5 commits April 21, 2025 09:48

Update src/System.Management.Automation/engine/CommandCompletion/Comp…

0a853cf

…letionHelpers.cs Co-authored-by: Ilya <darpa@yandex.ru>

Update src/System.Management.Automation/engine/CommandCompletion/Comp…

babbde7

…letionHelpers.cs Co-authored-by: Ilya <darpa@yandex.ru>

Update src/System.Management.Automation/engine/CommandCompletion/Comp…

002f46a

…letionHelpers.cs Co-authored-by: Ilya <darpa@yandex.ru>

Update src/System.Management.Automation/engine/CommandCompletion/Comp…

542a28a

…letionHelpers.cs Co-authored-by: Ilya <darpa@yandex.ru>

Add IgnoreCase to match strategy delegate names

7125ec9

iSazonov approved these changes Apr 21, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

iSazonov merged commit 92c6f7a into PowerShell:master Apr 21, 2025
37 of 40 checks passed

Sysoiev-Yurii pushed a commit to Sysoiev-Yurii/PowerShell that referenced this pull request May 12, 2025

Add single/double quote support for Join-String Argument Completer (P…

4f8d649

…owerShell#25283)

Add single/double quote support for Join-String Argument Completer #25283

Add single/double quote support for Join-String Argument Completer #25283

Uh oh!

Conversation

ArmaanMcleod commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Uh oh!

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!

ArmaanMcleod commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov commented Apr 10, 2025

Uh oh!

ArmaanMcleod commented Apr 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov commented Apr 19, 2025

Uh oh!

ArmaanMcleod commented Apr 19, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov commented Apr 20, 2025

Uh oh!

azure-pipelines bot commented Apr 20, 2025

Uh oh!

iSazonov 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!

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!

Uh oh!

Uh oh!

iSazonov commented Apr 20, 2025

Uh oh!

ArmaanMcleod commented Apr 21, 2025

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

microsoft-github-policy-service bot commented Apr 21, 2025 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Add single/double quote support for `Join-String` Argument Completer #25283

Add single/double quote support for `Join-String` Argument Completer #25283

ArmaanMcleod commented Apr 5, 2025 •

edited

Loading

ArmaanMcleod commented Apr 9, 2025 •

edited

Loading

microsoft-github-policy-service bot commented Apr 21, 2025 •

edited by unfurl-links bot

Loading