Refactor and add comments to CompletionRequiresQuotes to clarify implementation
#25223
Conversation
src/System.Management.Automation/engine/CommandCompletion/CompletionHelpers.cs
Outdated
Show resolved
Hide resolved
| // Check the type of the first token: | ||
| // - String token (e.g. a string literal or text) | ||
| // - PowerShell keyword (e.g. "while", "if") | ||
| bool isStringToken = firstToken is StringToken; | ||
| bool isKeywordToken = (firstToken.TokenFlags & TokenFlags.Keyword) != 0; | ||
|
|
||
| // Perform additional checks before checking characters: | ||
| // - If no quotes required and the first token is a string token | ||
| // - If the input is exactly 2 tokens long and the first token is a keyword | ||
| if ((!requireQuote && isStringToken) || (tokens.Length == 2 && isKeywordToken)) |
There was a problem hiding this comment.
Really these both comments are obvious and it is better to remove its.
Examples of non-obvious scenarios could be useful here, but it is better to show them in the general description of this method. If you don't know an exhaustive list, then it's best not to add anything.
There was a problem hiding this comment.
@iSazonov Yep good point, I agree. I have summarized this in the XML summary. I think code changes alone make intent clearer anyways.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
iSazonov
added
the
CL-CodeCleanup
Head branch was pushed to by a user without write access
|
@ArmaanMcleod What are your plans? It may be worth continuing PR with improving the interface. |
@iSazonov I think after this one I am happy with the refactoring and will come back to the interface. Code is cleaned up quite a bit and we have streamlined methods with |
|
@iSazonov I think auto merge didn't work for this one. Some checks still waiting. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
📣 Hey @ArmaanMcleod, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
2 participants
PR Summary
This PR simplifies
CompletionRequiresQuotesmethod and adds XML comments clarifying implementation with the tokenising and error handling. It can be difficult to understand what this method is trying to do, so hoping comments explain intent clearer. I have also broken-down conditions into variables so it's easier to follow flow and perhaps add to this method in future.PR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.- [ ] Issue filed:
(which runs in a different PS Host).