Fix various about topic help issues #4014
Conversation
* Fix double help topic printing issue * Fix regressions introduced by change a52adcd * Use wildcardPattern to fix tab completion
|
@powercode I changed some code CompletionCompleters.cs which was modified by you recently. Can you please have a look? |
|
signed off |
|
@PowerShell/powershell-maintainers This PR is ready to merge. |
| #if !CORECLR | ||
| if (returnValue != null) | ||
| { | ||
| returnValue = Path.GetDirectoryName(returnValue); |
There was a problem hiding this comment.
This is not needed even for FullCLR if you use Utils.GetApplicationBase(Utils.DefaultPowerShellShellID). It returns the pshome path.
There was a problem hiding this comment.
GetMshDefaultInstallationPath had only one reference, replaced where it was called with Utils.GetApplicationBase(Utils.DefaultPowerShellShellID); And deleted the function GetMshDefaultInstallationPath
| { | ||
| files = Directory.GetFiles(dirPath, wordToComplete); | ||
| var wildcardPattern = WildcardPattern.Get(wordToComplete, WildcardOptions.IgnoreCase); | ||
| files = Directory.GetFiles(dirPath).Where(f => wildcardPattern.IsMatch(Path.GetFileName(f))).ToArray(); |
There was a problem hiding this comment.
It's recommended to avoid LINQ in perf-sensitive code path (see pref-considerations). Tab completion is perf sensitive, so could you please replace this with a loop?
| // We don't actually need the drive, but the drive must be "mounted" in PowerShell before completion | ||
| // can succeed. This call will mount the drive if it wasn't already. | ||
| executionContext.SessionState.Drive.GetAtScope(wordToComplete.Substring(0, 1), "global"); | ||
| context.ExecutionContext.SessionState.Drive.GetAtScope(wordToComplete.Substring(0, 1), "global"); |
There was a problem hiding this comment.
You don't need to replace executionContext with context.ExecutionContext at the two spots here.
| var wordToComplete = context.WordToComplete + "*"; | ||
| var topicPattern = WildcardPattern.Get("about_*.help.txt", WildcardOptions.IgnoreCase); | ||
| string[] files = null; | ||
| ArrayList files = new ArrayList(); |
There was a problem hiding this comment.
Could you please use List<string> here? We prefer to strongly typed generic collection whenever possible, it will avoid a cast from object to string in the foreach (string file in files) loop later. However, the concern here is not about the perf hit of the extra casting, but that people will copy pattern of existing code, so better to let them copy List<string> instead of ArrayList.
|
Opened issue #4037 as discussed for refactoring. |
|
@adityapatwardhan If you search |
|
@daxian-dbw Fixed. |
4 participants
Fixes #3782 #2565 #4011 #4013