Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add completion single/double quote support for -Noun parameter #24977

Merged
merged 9 commits into from
Feb 11, 2025

Conversation

ArmaanMcleod
Copy link
Contributor

@ArmaanMcleod ArmaanMcleod commented Feb 9, 2025

PR Summary

Use CompletionCompleters.GetMatchingResults in NounArgumentCompleter so -Noun parameter for Get-Command has support for single/double quotes.

Also made the following improvements:

  • When -Verb is used, only get nouns which are possible with the following verbs.
  • use SortedSet<string> with StringComparer.IgnoreCase to ensure all nouns are unique and sorted and case insensitive. Also removed previous LINQ Order() call with this change.
  • Wrap Get-Command logic into CompletionCompleters.GetCommandInfo method. This can probably be used elsewhere and potentially reduce duplication of command querying code in completers. Also add using for creating powershell instance so its automatically disposed.

I also added some tab completion tests which seem to be missing for this completer.

PR Context

Related to #24873

PR Checklist

@iSazonov
Copy link
Collaborator

iSazonov commented Feb 9, 2025

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@iSazonov
Copy link
Collaborator

iSazonov commented Feb 9, 2025

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@iSazonov
Copy link
Collaborator

@ArmaanMcleod Please look failed tests. It seems previously had fake ordering for CompletionResult type.

@ArmaanMcleod
Copy link
Contributor Author

@iSazonov It looks like tests need to exclude Gridview cmdlets for Mac and Linux, hench test failures. These cmdlets only available on Windows.

I will fix this.

@iSazonov
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@ArmaanMcleod
Copy link
Contributor Author

@iSazonov I think it should be fine now. Thanks for the help.

I ran tests on Windows and Linux and it seems to be fine, hoping CI lets it go through 😄.

@@ -1724,7 +1724,7 @@ public IEnumerable<CompletionResult> CompleteArgument(
private static SortedSet<string> GetCommandNouns(IDictionary fakeBoundParameters)
{
Collection<CommandInfo> commands = CompletionCompleters.GetCommandInfo(fakeBoundParameters, "Module", "Verb");
SortedSet<string> nouns = new(StringComparer.OrdinalIgnoreCase);
SortedSet<string> nouns = new();
Copy link
Collaborator

@iSazonov iSazonov Feb 10, 2025

Choose a reason for hiding this comment

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

If cmdlet names are case-insensetive on all OS-s this must be case-insensetive too. I think we should think about fixing tests only.

Copy link
Contributor Author

@ArmaanMcleod ArmaanMcleod Feb 10, 2025

Choose a reason for hiding this comment

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

Yep I agree, although to completer it does not make a difference. I have updated code & test. Only way is to use sorted set in the test becauseSort-Object -Unique gives different ordering on Linux. I think this is fine because it is an accurate comparison anyways.

@iSazonov

This comment was marked as outdated.

This comment was marked as outdated.

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 11, 2025
@iSazonov iSazonov self-assigned this Feb 11, 2025
@iSazonov iSazonov merged commit 0f7ba43 into PowerShell:master Feb 11, 2025
39 of 41 checks passed
Copy link
Contributor

microsoft-github-policy-service bot commented Feb 11, 2025

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

🔗 https://aka.ms/PSRepoFeedback

@daxian-dbw daxian-dbw mentioned this pull request Feb 21, 2025
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants