Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFix parsing the -Settings object as a path when the path object originates from an expression #915
Conversation
…sion that is not fully evaluated yet.
|
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 |
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Mar 2, 2018
Member
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 |
This comment has been minimized.
This comment has been minimized.
| try { | ||
| $initialLocation = Get-Location | ||
| Set-Location $PSScriptRoot | ||
| $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'gci' -Settings (Join-Path (Get-Location).Path '.\SettingsTest\..\SettingsTest\Project1\PSScriptAnalyzerSettings.psd1') |
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Mar 2, 2018
Member
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.
This comment has been minimized.
This comment has been minimized.
bergmeister
Mar 3, 2018
Author
Collaborator
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'.
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Mar 4, 2018
Member
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.
This comment has been minimized.
This comment has been minimized.
bergmeister
Mar 4, 2018
•
Author
Collaborator
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,
This comment has been minimized.
This comment has been minimized.
bergmeister commentedMar 1, 2018
•
edited
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
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