build.ps1/build_helpers.ps1 issues

[msftrncs]

trafficstars

Issue Description

build.ps1

• comment based help attempt to include an argument to the example statement, this causes PowerShell to invalidate the comment based help entirely. https://github.com/PowerShell/EditorSyntax/blob/cf27d6e54ff9fc593bfb46d22a671898fb31c0a9/build.ps1#L10
• [cmdletbinding()] and [parameter()] attributes are unnecessary. https://github.com/PowerShell/EditorSyntax/blob/cf27d6e54ff9fc593bfb46d22a671898fb31c0a9/build.ps1#L28-L33
• use of & (call operator) to execute commands with an unquoted (and non-expanded) name are unnecessary. (3 occurrences total) https://github.com/PowerShell/EditorSyntax/blob/cf27d6e54ff9fc593bfb46d22a671898fb31c0a9/build.ps1#L41

tools\build_helpers.ps1

function ExtractAtom

• doesn't support UNC paths due to $PSScriptRoot and Resolve-Path will format with the filesystem:: provider, which notation is not acceptable to [System.IO.Compression.ZipFile]::ExtractToDirectory() method. This is informational only, due to the fact that UNC paths do not work for NPM anyway.

function ParseJasmine

https://github.com/PowerShell/EditorSyntax/blob/cf27d6e54ff9fc593bfb46d22a671898fb31c0a9/tools/build-helpers.ps1#L38-L56

• first line starts with using $string parameter, the rest use $_. Surprisingly it works, but only because in RunSpecs the call to ParseJasmine is part of a ForEach-Object which populates the $_ automatic variable.
• the ^\s+Expected replacement doesn't seem to be working correctly. Maybe Atom/Atom-Grammar-Test has changed? I think 'to equal' now is to be 'instead found'.
• this function should really be a filter as it might provide a better fit. Note, also demonstrating the 'switch' statement with -regex parameter:

  filter ParseJasmine {
      switch -regex ($_) {
          ^\s+at {
              ''
              break
          }
  
          ^\s+it {
              $_ -replace '^(\s+)(it)', '$1[-] It'
              break
          }
  
          ^\s+Expected {
              $_ -replace '^(\s*)(Expected.*?)\s(instead found .*)', "`$1`$2`n`$1`$3"
              break
          }
  
          default {
              $_
          }
      }
  }
  

  This requires changing RunSpecs: https://github.com/PowerShell/EditorSyntax/blob/cf27d6e54ff9fc593bfb46d22a671898fb31c0a9/tools/build-helpers.ps1#L73 to read more like:

          & $script:ATOM_EXE_PATH --test $specpath *>&1 | ParseJasmine
  

function RunSpecs

• quotes around the variable $script:ATOM_EXE_PATH are unneeded, probably old habit from CMD or another shell? (seen below)
• test path 'specs' is literal on ATOM command, but yet in a variable $specpath in a test previously. Only issue with using the variable is a chance to get the PowerShell filesystem:: provider notation injected. https://github.com/PowerShell/EditorSyntax/blob/cf27d6e54ff9fc593bfb46d22a671898fb31c0a9/tools/build-helpers.ps1#L72-L73

I'll post a PR shortly demonstrating these changes.

[msftrncs]

Realized I missed a minor detail:

Originally:

$x = $_ -replace '^(\s*)(Expected)(.*)\s(to equal .*)','$1$2$3%%$1$4' 
$x.Replace('%%',"`n  ") 

I replaced with:

$_ -replace '^(\s*)(Expected.*?)\s(instead found .*)', "`$1`$2`n`$1`$3"

I missed the two spaces following the `n in the string .replace() method invocation. Ideally these should be stuck between `$1 and `$3 incase $1 results in a pattern of `t. They cause the instead found message to be slightly indented compared to the Expected message, but I am not sure this is too important.