WIP: restore GetCommandInfo to non-obsolete status #953
Conversation
There was a problem hiding this comment.
Overall looks OK but I have 2 questions that I left in the comments to do with the removal of some functionality
- Can we just remove the aliasing lookup?
- If the
commandInfoCachevariable can be removed (why?), it should be removed entirely as I could not find any references to it.
| } | ||
|
|
||
| // check if it is an alias | ||
| string cmdletName = Helper.Instance.GetCmdletNameFromAlias(name); |
There was a problem hiding this comment.
Why can we just remove the alias lookup?
| } | ||
|
|
||
| var commandInfo = GetCommandInfoInternal(cmdletName, commandType); | ||
| commandInfoCache.Add(cmdletName, commandInfo); |
There was a problem hiding this comment.
If we do not add to the commandInfoCache any more (not sure why we can remove it or why it was there in the first place?), then we should remove this variable entirely since there do not seem to be any other references to it.
There was a problem hiding this comment.
that is a big error - I meant to leave it in the Internal routine (where it belongs) - updated PR on the way
Add overload for GetCommandInfo without CommandType to match general usage Remove GetCommandInfoLegacy method
Previous implementation of command cache usage would take the lock during the pipeline execution of Get-Command which could be very long. This breaks up the lock to not include the pipeline invocation.
JamesWTruher
force-pushed
the
jameswtruher/removeobsoletecalls
branch
from
43f865c to
d59d931
Compare
|
I'm not sure what's going on here and why there's a failure - I haven't been able to reproduce the test failures. Still Investigating |
|
You got at least further than me. I'll take a look at it and see if I can do something. I should note that I do not trust AppVeyor completely. I have 9 tests that have always failed locally using PowerShell Core only (on Windows or Ubuntu) and I will open an issue or PR soon with details about that. Also, I have had cases with PSSA where one test file could cause a test failure in another test file... |
…hat no pollution has occurred
| @@ -38,6 +38,7 @@ test_script: | |||
| - ps: | | |||
| if ($env:PowerShellEdition -eq 'WindowsPowerShell') { | |||
| $modulePath = $env:PSModulePath.Split([System.IO.Path]::PathSeparator) | Where-Object { Test-Path $_} | Select-Object -First 1 | |||
| if ( Test-Path "$modulePath\PSScriptAnalyzer" ) { Remove-Item -recurse -force "$modulePath\PSScriptAnalyzer" } | |||
There was a problem hiding this comment.
Did you find that there was a PSScriptAnalyzer folder at some point? All 4 builds should run on fresh images (as if it was a new VM), therefore it would be surprising to me to find PSSA already being there.
JamesWTruher
force-pushed
the
jameswtruher/removeobsoletecalls
branch
from
51865e4 to
89811c8
Compare
…all we should really check is the rule violation
JamesWTruher
force-pushed
the
jameswtruher/removeobsoletecalls
branch
from
89811c8 to
9ab158f
Compare
JamesWTruher
changed the title
|
I'm going to close this and revisit it later |
Add overload for GetCommandInfo without CommandType to match general usage
Remove GetCommandInfoLegacy method
PR Summary
GetCommandInfo had been written in a way that the command type was never used. A previous PR solved this by creating a legacy method and marking it obsolete and using a new API. This PR changes this behavior to codify what had been present in the case that no CommandType parameter was passed, which was to search all possible command types.
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets. Please mark anything not applicable to this PRNA.WIP:to the beginning of the title and remove the prefix when the PR is ready