PoshCode / Configuration

A module to help other modules have settings
MIT License
176 stars 27 forks source link

Improve compatibility via Parser method change #7

Closed RamblingCookieMonster closed 7 years ago

RamblingCookieMonster commented 7 years ago

Hi Joel!

It looks like the four parameter ParseInput method was introduced in PowerShell 5, see #6 for more info - swapped this out for the three parameter method.

Not sure if there's more to be done, but superficially seems to work.

Cheers!

lzybkr commented 7 years ago

You'll get better error messages (file w/ line) if you use the ParseFile api. I'd call ParseFile if it's a file, ParseInput if it's not.

RamblingCookieMonster commented 7 years ago

I'm out of my league, but is there ever a logic path where this will hit anything but a file? In particular, it would seem this validation would kill any chances of that.

If it makes sense and you're up for it @Jaykul, I'll strip out the test-path (assumption: ValidateScript has already validated it's a file) and just call ParseFile

RamblingCookieMonster commented 7 years ago

Oh, not a .NET guy, just noticed IO.Path isn't going to validate anything on the file system. Would it make sense to include a Test-Path in the validatescript, or keep that in the function, e.g. if(test-path){do stuff} else {fail}?

Jaykul commented 7 years ago

In the rest of the module I'm very explicit about this distinction. E.g.:

      if(Test-Path $InputObject -ErrorAction SilentlyContinue) {
         $AST = [System.Management.Automation.Language.Parser]::ParseFile( (Convert-Path $InputObject), [ref]$Tokens, [ref]$ParseErrors)
         $ScriptRoot = Split-Path $InputObject
      } else {
         $ScriptRoot = $PoshCodeModuleRoot
         $OFS = "`n"
         # Remove SIGnature blocks, PowerShell doesn't parse them in .psd1 and chokes on them here.
         $InputObject = "$InputObject" -replace "# SIG # Begin signature block(?s:.*)"
         $AST = [System.Management.Automation.Language.Parser]::ParseInput($InputObject, [ref]$Tokens, [ref]$ParseErrors)
      }

This function was originally written separately as part of a build script, so it (obviously) was a little sloppier than the rest of the module code.

Anyway. I would say that yes, [IO.Path]::GetExtension($_) -ne ".psd1" essentially requires that the $path be an actual path (and not just content that happens to end in .psd1) --even though the function has a couple of lines attempting to deal with content instead of paths-- so changing this to use ParseFile makes more sense than changing it to use the other overload of ParseInput, and we should just make sure to call Convert-Path. You can add a Test-Path to the validation if you want, but Convert-Path will fail convincingly anyway ...

Jaykul commented 7 years ago

Any chance you'll fix that @RamblingCookieMonster? If not, I can take your change and fix it.

RamblingCookieMonster commented 7 years ago

Sure, can revisit, might be a week or two, tied up at a conference - completely forgot about this, thanks for the reminder!

Cheers!

Jaykul commented 7 years ago

Merged by hand.