Add `QuoteCompletionText` method to CompletionHelpers class #25180

ArmaanMcleod · 2025-03-15T04:04:11Z

PR Summary

Add QuoteCompletionText method to CompletionHelpers class.

This is to ensure quoted completions also get escaping done before surrounding quotes are added. Current code in GetMatchingResults just surrounds completion with quotes without doing any escaping.

I've replaced this code as well as many single quote escaping calls in CompletionCompleters.

PR Context

PR Checklist

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

ArmaanMcleod · 2025-03-15T14:48:37Z

Thanks for feedback @iSazonov. I have made method more flexible with different escaping parameters, a lot of code in CompletionCompleters can use new method. Mostly the simple blocks where single quote escaping was happening before quoting. I've also added XUnit tests for the QuoteCompletionText method.

iSazonov · 2025-03-15T16:27:39Z

a lot of code in CompletionCompleters can use new method.

Can we use it in GetMatchingResults() method for other completers?

ArmaanMcleod · 2025-03-15T22:36:51Z

a lot of code in CompletionCompleters can use new method.

Can we use it in GetMatchingResults() method for other completers?

@iSazonov It is used in GetMatchingResults:

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

Line 41 in 6ea3b85

string completionText = QuoteCompletionText(value, quote);

ArmaanMcleod · 2025-03-15T23:02:34Z

Probably best to also wait on #25184 so existing implementation of GetMatchingResults covered by test.

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

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

ArmaanMcleod · 2025-03-16T08:52:46Z

I'll fix the tests.

ArmaanMcleod · 2025-03-17T09:09:19Z

@MartinGC94 If you have time, it would be great to have a review. I have started writing a new method QuoteCompletionText to consolidate escaping + quoting into a single method.

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

…n-text

iSazonov · 2025-03-17T11:25:20Z

/azp run

azure-pipelines · 2025-03-17T11:25:33Z

Azure Pipelines successfully started running 1 pipeline(s).

MartinGC94 · 2025-03-17T12:09:57Z

@ArmaanMcleod At a glance it looks fine to me for the intended purpose of removing all the duplicate code and escaping single quotes.

iSazonov · 2025-03-17T17:02:00Z

@ArmaanMcleod Please look CI fails.

…n-text

ArmaanMcleod · 2025-03-19T08:55:27Z

Thanks @iSazonov for review and suggestions. I have added back in dynamic escaping for double quotes and updated tests.

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

This reverts commit a42cad8.

This reverts commit ff33f9c.

This reverts commit d42df5a.

iSazonov · 2025-03-19T11:59:24Z

test/xUnit/csharp/test_CompletionHelpers.cs

+        [InlineData("", "'", "''")]
+        [InlineData("", "\"", "\"\"")]
+        [InlineData("'", "'", "''''")]
+        [InlineData("", "", "''")]


I still think we should cover CompletionRequiresQuotes by tests too. There we use Parser and additionally checks for backtick and $.
So string for tests could be like "a b", "$a", "a" Maybe you could think more tests - I see StringTokenandTokenFlags.Keyword` in the method. The test strings could be "a$a" and "while" accordingly with variants with backtick and $.

@iSazonov Added some tests here for CompletionRequiresQuotes: fced8d7

Thanks!

The QuoteCompletionText is more interesting for us. Maybe add 2-3 key tests here?

@iSazonov Ok I have added more tests and reorganized them in 9bff47b.

test/xUnit/csharp/test_CompletionHelpers.cs

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

iSazonov · 2025-03-19T16:09:06Z

test/xUnit/csharp/test_CompletionHelpers.cs

+        [InlineData("", "'", "''")]
+        [InlineData("", "\"", "\"\"")]
+        [InlineData("'", "'", "''''")]
+        [InlineData("", "", "''")]


Thanks!

The QuoteCompletionText is more interesting for us. Maybe add 2-3 key tests here?

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

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

…n-text

test/xUnit/csharp/test_CompletionHelpers.cs

iSazonov · 2025-03-22T08:53:35Z

test/xUnit/csharp/test_CompletionHelpers.cs

+        [InlineData("word with space", "'", "'word with space'")]
+        [InlineData("word with space", "\"", "\"word with space\"")]
+        [InlineData("word\"with\"quotes", "'", "'word\"with\"quotes'")]
+        [InlineData("word'with'quotes", "\"", "\"word'with'quotes\"")]


I'd add tests for "while", "$variable" and "key$word".

@iSazonov I believe we have tests for those already?

Or do we want the same test from CompletionRequiresQuotes in QuoteCompletionText?

It looks like duplicate but it protects from regression if we will improve CompletionRequiresQuotes and change its tests.

Yep makes sense, I have added tests in 5cb39e0

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

microsoft-github-policy-service · 2025-03-22T13:05:23Z

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

🔗 https://aka.ms/PSRepoFeedback

Microsoft Forms

Add QuoteCompletionText method to CompletionHelpers class

6e96b45

iSazonov reviewed Mar 15, 2025

View reviewed changes

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

ArmaanMcleod added 7 commits March 16, 2025 01:14

Include more single/double quote handling parameters in method

14c032c

Replace simple single quoting calls in CompletionHelpers with new method

f58947e

undo unrelated formatting change

ae9fc5a

Add Xunit tests

489a383

Add tests for QuoteCompletionText

a46f061

Add XML documentation

f836deb

Code Factor: Fix preceeding spaces

6ea3b85

ArmaanMcleod requested a review from iSazonov March 15, 2025 14:47

iSazonov reviewed Mar 16, 2025

View reviewed changes

ArmaanMcleod and others added 3 commits March 16, 2025 19:06

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

7808686

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

Remove e.g. from descriptions

ad430df

Remove extra parameters which are not used

6d6ab3c

ArmaanMcleod requested a review from iSazonov March 16, 2025 08:22

ArmaanMcleod added 3 commits March 16, 2025 20:28

Modify tests for just single quote char parameter

5d85fa4

Use CodeGeneration.EscapeSingleQuotedStringContent instead of Replace

df6d172

Replace ProcessParameter single quote escaping

3d2a3e1

ArmaanMcleod commented Mar 17, 2025

View reviewed changes

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

Merge branch 'master' into completion-helpers-include-quote-completio…

56771a6

…n-text

Merge branch 'master' into completion-helpers-include-quote-completio…

45db39c

…n-text

ArmaanMcleod requested a review from iSazonov March 19, 2025 08:54

ArmaanMcleod added 2 commits March 19, 2025 20:40

Use simple if/else if instead of switch

ff33f9c

Use ternary operator

a42cad8

iSazonov reviewed Mar 19, 2025

View reviewed changes

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

ArmaanMcleod added 4 commits March 19, 2025 22:02

Revert "Use ternary operator"

ef5e611

This reverts commit a42cad8.

Revert "Use simple if/else if instead of switch"

0608ba2

This reverts commit ff33f9c.

Revert "Add double quote escaping back in and updated tests"

6d14827

This reverts commit d42df5a.

Add test with empty string and quote

543c043

ArmaanMcleod requested a review from iSazonov March 19, 2025 11:06

iSazonov reviewed Mar 19, 2025

View reviewed changes

Add tests for CompletionRequiresQuotes

fced8d7

ArmaanMcleod requested a review from iSazonov March 19, 2025 12:20

iSazonov reviewed Mar 19, 2025

View reviewed changes

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

Update test/xUnit/csharp/test_CompletionHelpers.cs

2ab3316

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

iSazonov reviewed Mar 19, 2025

View reviewed changes

ArmaanMcleod and others added 2 commits March 20, 2025 06:42

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

6f62999

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

Added more tests

9bff47b

ArmaanMcleod requested a review from iSazonov March 22, 2025 01:51

Merge branch 'master' into completion-helpers-include-quote-completio…

a70a82d

…n-text

iSazonov reviewed Mar 22, 2025

View reviewed changes

ArmaanMcleod and others added 2 commits March 22, 2025 19:55

Update test/xUnit/csharp/test_CompletionHelpers.cs

d546e0e

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

Add tests for special variables in TestQuoteCompletionText

5cb39e0

iSazonov approved these changes Mar 22, 2025

View reviewed changes

iSazonov self-assigned this Mar 22, 2025

This comment was marked as outdated.

Sign in to view

iSazonov merged commit 94d0361 into PowerShell:master Mar 22, 2025
36 of 39 checks passed

Add QuoteCompletionText method to CompletionHelpers class #25180

Add QuoteCompletionText method to CompletionHelpers class #25180

Uh oh!

Conversation

ArmaanMcleod commented Mar 15, 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!

ArmaanMcleod commented Mar 15, 2025

Uh oh!

iSazonov commented Mar 15, 2025

Uh oh!

ArmaanMcleod commented Mar 15, 2025

Uh oh!

ArmaanMcleod commented Mar 15, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ArmaanMcleod commented Mar 16, 2025

Uh oh!

ArmaanMcleod commented Mar 17, 2025

Uh oh!

Uh oh!

iSazonov commented Mar 17, 2025

Uh oh!

azure-pipelines bot commented Mar 17, 2025

Uh oh!

MartinGC94 commented Mar 17, 2025

Uh oh!

iSazonov commented Mar 17, 2025

Uh oh!

ArmaanMcleod commented Mar 19, 2025

Uh oh!

Uh oh!

iSazonov Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add `QuoteCompletionText` method to CompletionHelpers class #25180

Add `QuoteCompletionText` method to CompletionHelpers class #25180

ArmaanMcleod commented Mar 15, 2025 •

edited

Loading

iSazonov Mar 19, 2025 •

edited

Loading

microsoft-github-policy-service bot commented Mar 22, 2025 •

edited by unfurl-links bot

Loading