Write proper error messages for Get-Command ' ' #13564
Conversation
|
|
|
I see now that an alternate and maybe cleaner solution would involve changing only this line |
| // Home Path: "~\command.exe" | ||
| // Drive Relative Path: "\Users\User\AppData\Local\Temp\command.exe" | ||
| if (_commandName[0] == '.' || _commandName[0] == '~' || _commandName[0] == '\\') | ||
| if (!string.IsNullOrEmpty(_commandName) && (_commandName[0] == '.' || _commandName[0] == '~' || _commandName[0] == '\\')) |
There was a problem hiding this comment.
_commandName is defined as non-nullable so we can do not check null.
Also it is on a hot path and we could optimize the code a bit. I suggest to add && _commandName.Length != 0 in if above, define char firstChar = _commandName[0] and use the firstChar in the line.
There was a problem hiding this comment.
@iSazonov just to confirm. Is this along the lines of what you're suggesting?
char firstChar = _commandName[0];
if (_commandName.Length != 0 && (firstChar == '.' || firstChar == '~' || firstChar == '\\'))
I agree it would be more optimized but the issue of this PR (Get-Command ' ' returns the wrong exception) wouldn't be fixed. Sorry I may be misunderstanding. I'm still learning.
There was a problem hiding this comment.
(!string.IsNullOrEmpty(_commandName)
You added the check to fix the issue. But _commandName is annotated as non-nullable and we can exclude null check and have _commandName.Length != 0. And the check should be in previous if condition (before char firstChar = _commandName[0]).
| It "Returns CommandNotFoundException if a single space is used" { | ||
| {Get-Command ' ' -ErrorAction Stop} | Should -Throw -ErrorId "CommandNotFoundException,Microsoft.PowerShell.Commands.GetCommandCommand" | ||
| } |
There was a problem hiding this comment.
For consistency we could add tests for null and empty arguments. Please use -TestCases.
I think it makes no sense to create new variable - this complicates code and don't protect underlying internal methods. |
| //char firstChar = _commandName[0]; | ||
| if (_commandName.Length != 0) |
There was a problem hiding this comment.
Please remove commented code and move the condition in line 1104 to line 1090.
There was a problem hiding this comment.
Oh yeah thanks. Can't believe I missed that.
test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Command.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Command.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Command.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Command.Tests.ps1
Outdated
Show resolved
Hide resolved
Co-authored-by: Ilya <darpa@yandex.ru>
|
@SteveL-MSFT what do you think of the added test cases? Thanks. |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
@adityapatwardhan I don't mean to bother you but I just wanted to know if there's anything else I can do to help get this PR approved? Thanks. |
|
Currently working on some high priority items. I will be reviewing this after. Thank you for your patience. |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
@SteveL-MSFT can you update your review |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
@jakekerr Thanks for your contribution!< 4E30 /p> |
|
🎉 Handy links: |
PR Summary
Fix #13342. Checks if _commandName is empty or null in the DoPowerShellRelativePathLookup() method. Removes a Dbg.Assert() that checks if the name is empty or null. The assertion fails because _commandName is trimmed in setupPathSearcher().
PR Context
After this change
Get-Command ' 'will return a CommandNotFoundException instead of a IndexOutOfRangeException. The Dbg.Assert was removed because according to Issue #10165 whitespace should be a valid argument for Get-Command.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.