KEMBAR78
Use Get-Help approach to find 'about_*.help.txt' files with correct locale for completions by MartinGC94 · Pull Request #24194 · PowerShell/PowerShell · GitHub
Skip to content

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
@azure-pipelines

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.

@azure-pipelines

This comment was marked as outdated.

@iSazonov

This comment was marked as outdated.

@azure-pipelines

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
@microsoft-github-policy-service
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