Skip to content
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

Conversation

bergmeister
Copy link
Collaborator

@bergmeister bergmeister commented Mar 1, 2018

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 PR NA.

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • [NA] User facing documentation needed
  • Change is not breaking
  • Make sure you've added a new test if existing tests do not effectively test the code changed
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

Copy link
Member

@JamesWTruher JamesWTruher left a 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
Copy link
Member

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
Copy link
Member

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')
Copy link
Member

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.

Copy link
Collaborator Author

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'.

Copy link
Member

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.

Copy link
Collaborator Author

@bergmeister bergmeister Mar 4, 2018

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,

Copy link
Member

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.

@JamesWTruher JamesWTruher merged commit bc5b2dd into PowerShell:development Mar 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Settings file cannot be found in a special case when specifying -Setting parameter object in a certain way
2 participants