-
Notifications
You must be signed in to change notification settings - Fork 383
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
Fix parsing the -Settings object as a path when the path object originates from an expression #915
Fix parsing the -Settings object as a path when the path object originates from an expression #915
Conversation
…sion that is not fully evaluated yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you validated that the new test actually hits the new code? I'm not sure it does
It "Should process relative settings path even when settings path object is an expression" { | ||
try { | ||
$initialLocation = Get-Location | ||
Set-Location $PSScriptRoot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be Push-Location
, that way you don't need to save the location at all.
$warnings.Count | Should -Be 1 | ||
} | ||
finally { | ||
Set-Location $initialLocation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pop-Location
try { | ||
$initialLocation = Get-Location | ||
Set-Location $PSScriptRoot | ||
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'gci' -Settings (Join-Path (Get-Location).Path '.\SettingsTest\..\SettingsTest\Project1\PSScriptAnalyzerSettings.psd1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this - in this case, the execution of Join-Path happens before the cmdlet is ever called, so this is just result of the join-path
rather than an actual expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if it is really Join-Path
because (Join-path a b).GetType()
returns a string. As far as I understood when stepping through it in the debugger is that because we are in BeginProcessing()
, the passed in object is not resolved to a string yet but the code was assuming that in the case of files that the input can be casted to a string in order to determine that it is of type 'file'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the place to look is in the setter of the parameter value. You could set a breakpoint in the setter (around line 215 of InvokeScriptAnalyzerCommand.cs) and inspect the value (or Settings
after it's been set). The way powershell should be doing this is because of the parens, that statement is evaluated and then the parameter is bound to the string output of join-path
. I haven't walked this through the debugger, but the way to check is that if Settings is the string result of the join-path
(rather than the whole statement with parens) the check above should be definitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right that PowerShell evaluates it in the setter but the object is of type System.Management.Automation.PSObject
, whereas our property is just System.Object
and therefore .Net truthfully says that it is not of type string. What I could do is try to cast it to PSObject
then and check whether its BaseObject
property is of type string,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it winds up being not a string, that's good enough.
PR Summary
Fixes #914
The referenced bug has been found independently of the recent improvement of supporting relative paths and is also independent of it.
The code which determines the type of setting object (path/hashtable) needed tweaking, see referenced issue.
If the passed in object comes from an expression that evaluates to a string, it needs to be casted to PSObject first and then the check needs to be on the BaseObject property whether it is of type string.
Error handling also got improved to not proceed if the settings type could not be determined instead of continuing.
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
x
between 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