Skip to content

Use Get-Help approach to find 'about_*.help.txt' files with correct locale for completions #24194

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

Merged
merged 5 commits into from
Feb 27, 2025

Conversation

MartinGC94
Copy link
Contributor

@MartinGC94 MartinGC94 commented Aug 22, 2024

PR Summary

Updates the help topic completion code to use the same methods that Get-Help uses when finding the topics. This will make it more accurate when handling different locales and also makes it search modules for about_ topics.
I haven't added any tests as the Get-Help tests presumably already covers all the edge cases that are now fixed by this.

PR Context

PR Checklist

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Aug 29, 2024
Copy link

@Imran-imtiaz48 Imran-imtiaz48 left a comment

Choose a reason for hiding this comment

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

  1. Efficient Use of Generics and Collections:

    • The use of List<CompletionResult> ensures that the results are stored in a typed and efficient collection, improving performance and type safety.
    • The use of ArrayList helpProviders is straightforward, and the loop efficiently finds the right help provider by iterating in reverse order.
  2. Clear Purpose:

    • The function's intent to autocomplete help topics is clear. It retrieves the relevant help topics by searching for help files based on the user's input (context.WordToComplete).
    • The logic for filtering filenames that start with "about_" indicates a good understanding of the help topic conventions.
  3. Error Prevention:

    • The check fileName.StartsWith("about_", StringComparison.OrdinalIgnoreCase) prevents case-sensitivity issues, which is good for cross-platform consistency.

Suggestions for Improvement:

  1. Null Checks:

    • It might be helpful to include null checks for helpFileProvider. If no provider is found in the loop, helpFileProvider would remain null, potentially causing a NullReferenceException in helpFileProvider.GetExtendedSearchPaths().
    • Consider adding a check like:
      if (helpFileProvider == null)
      {
          // Handle the case where no HelpFileHelpProvider is found
          return results;
      }
  2. Refactor for Clarity:

    • The for loop iterating through helpProviders can be abstracted into a helper method or LINQ expression for better readability:
      var helpFileProvider = helpProviders.OfType<HelpFileHelpProvider>().LastOrDefault();
  3. Use More Descriptive Variable Names:

    • helpFileProvider is descriptive, but the variable i in the loop could be replaced with something more meaningful, like providerIndex. However, this might be less relevant if you switch to LINQ as suggested above.
  4. File Searching:

    • Instead of relying on the hard-coded string ".help.txt", you might want to store that as a constant or parameterize it to enhance maintainability.
      const string fileExtension = ".help.txt";
  5. Potential Optimization with String Manipulation:

    • In the line where you use fileName.Substring(0, fileName.LastIndexOf(".help.txt")), you could consider using Path.GetFileNameWithoutExtension(path) if you're always targeting the last extension.
  6. Asynchronous Operations:

    • Depending on the usage context, if this file search might involve I/O-bound operations that could block the main thread, consider making this method asynchronous:
      static async Task<List<CompletionResult>> CompleteHelpTopicsAsync(CompletionContext context)
  7. Logging and Error Handling:

    • Currently, there are no logging or exception-handling mechanisms in place. Adding error handling for file operations and logging unexpected behaviors would help with debugging and robustness.

Final Thoughts:

The method is well-structured and efficient for its intended use. With a few improvements around null checks, readability, and potential optimizations, it can become more robust and maintainable.

Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

Thanks Martin! Looks great, one question/potential change

MartinGC94 and others added 2 commits December 12, 2024 09:32
Co-authored-by: Patrick Meinecke <SeeminglyScience@users.noreply.github.com>
@iSazonov

This comment was marked as outdated.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Feb 26, 2025

This comment was marked as outdated.

@iSazonov iSazonov added Review - Needed The PR is being reviewed CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Feb 26, 2025
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

I found tests in TabCompletion.Test.ps1 so it is ok for tests.

@iSazonov iSazonov self-assigned this Feb 26, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Feb 26, 2025
@iSazonov

This comment was marked as outdated.

This comment was marked as outdated.

@iSazonov

This comment was marked as outdated.

This comment was marked as outdated.

@iSazonov
Copy link
Collaborator

iSazonov commented Feb 27, 2025

@MartinGC94 Please reword the PR header and description. I see the PR is exclusively for "about_" files.

@MartinGC94
Copy link
Contributor Author

I don't see what's wrong with the current title and description. It matches exactly what this PR does: It replaces the custom logic to find help topics in the completion code with the logic that the help system uses.

@iSazonov
Copy link
Collaborator

The header says nothing about "about_" files. And it follows from the description that the locale search has been fixed, and the rest is an addition. Maybe something like "Use Get-Help approach to find 'about_*.help.txt' files with correct locale"

@MartinGC94 MartinGC94 changed the title Use help system code to complete help topics Use Get-Help approach to find 'about_*.help.txt' files with correct locale for completions Feb 27, 2025
@iSazonov iSazonov merged commit bdf0400 into PowerShell:master Feb 27, 2025
39 of 41 checks passed
Copy link
Contributor

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

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

🔗 https://aka.ms/PSRepoFeedback

@MartinGC94 MartinGC94 deleted the UpdateHelpTopicCompletion branch February 28, 2025 10:40
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.

4 participants