Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

AddQuoteCompletionText method to CompletionHelpers class#25180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@ArmaanMcleod
Copy link
Contributor

@ArmaanMcleodArmaanMcleod commentedMar 15, 2025•
edited
Loading

PR Summary

AddQuoteCompletionText method to CompletionHelpers class.

This is to ensure quoted completions also get escaping done before surrounding quotes are added. Current code inGetMatchingResults 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

@ArmaanMcleod
Copy link
ContributorAuthor

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
Copy link
Collaborator

a lot of code in CompletionCompleters can use new method.

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

@ArmaanMcleod
Copy link
ContributorAuthor

a lot of code in CompletionCompleters can use new method.

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

@iSazonov It is used inGetMatchingResults:

stringcompletionText=QuoteCompletionText(value,quote);

@ArmaanMcleod
Copy link
ContributorAuthor

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

@ArmaanMcleod
Copy link
ContributorAuthor

I'll fix the tests.

@ArmaanMcleod
Copy link
ContributorAuthor

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

@iSazonov
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MartinGC94
Copy link
Contributor

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

ArmaanMcleod reacted with hooray emoji

@iSazonov
Copy link
Collaborator

@ArmaanMcleod Please look CI fails.

@ArmaanMcleod
Copy link
ContributorAuthor

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

[InlineData("","'","''")]
[InlineData("","\"","\"\"")]
[InlineData("'","'","''''")]
[InlineData("","","''")]
Copy link
Collaborator

@iSazonoviSazonovMar 19, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 seeStringTokenandTokenFlags.Keyword` in the method. The test strings could be "a$a" and "while" accordingly with variants with backtick and $.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@iSazonov Added some tests here for CompletionRequiresQuotes:fced8d7

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks!

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Up!

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@iSazonov Ok I have added more tests and reorganized them in9bff47b.

Co-authored-by: Ilya <darpa@yandex.ru>
[InlineData("","'","''")]
[InlineData("","\"","\"\"")]
[InlineData("'","'","''''")]
[InlineData("","","''")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks!

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

[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\"")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@iSazonov I believe we have tests for those already?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yep makes sense, I have added tests in5cb39e0

@iSazonoviSazonov self-assigned thisMar 22, 2025
@iSazonov

This comment was marked as outdated.

@azure-pipelines

This comment was marked as outdated.

@iSazonoviSazonov merged commit94d0361 intoPowerShell:masterMar 22, 2025
36 of 39 checks passed
@microsoft-github-policy-service
Copy link
Contributor

microsoft-github-policy-servicebot commentedMar 22, 2025•
edited by unfurl-linksbot
Loading

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

🔗https://aka.ms/PSRepoFeedback

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@iSazonoviSazonoviSazonov approved these changes

Assignees

@iSazonoviSazonov

Labels

CL-CodeCleanupIndicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ArmaanMcleod@iSazonov@MartinGC94
[8]ページ先頭
©2009-2025 Movatter.jp