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

opt(decompress): Add [Array] and pipeline support to Expand-XXX's params #3534

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

niheaven
Copy link
Member

@niheaven niheaven commented Jun 27, 2019

Now Expand-xxx functions accept [Array] params (Path, DestinationPath, ExtractDir), and work for pipeline params (aa, bb, cc | Expand-xxx or @(aa, bb, cc) | Expand-xxx.

No more ForEach-Object { Expand-xxx $_ }.

Useful for install script in manifests.

lib/decompress.ps1 Outdated Show resolved Hide resolved
lib/decompress.ps1 Outdated Show resolved Hide resolved
@Ash258
Copy link
Contributor

Ash258 commented Aug 23, 2019

@niheaven Could you also look into this?

{
    "version": "5.4.6.0",
    "url": "http://www.vieas.com/software/dl/Vieas5460_32.zip#/cosi",
    "pre_install": "\"$dir\\$fname\" | Expand-7zipArchive; 'WRONG'",
    "post_install": "\"$dir\\$fname\" | Expand-7zipArchive -DestinationPath $dir; 'OK'"
}

When using pipeline, default DestinationPath is not resolved as $Path is $null and I do not know why, as $Path is position 0 and set to ValueFromPipeline

image

@Ash258
Copy link
Contributor

Ash258 commented Aug 23, 2019

OK.Nevermind. This come from PowerShell precedence of execution.

Parameters are validated and binded before pipeline input processing.

image

This could be solved with removing default from $DestinationPath parameter and then do simple check in begin block or function body if(-not $DestinationPath) { $DestinationPath = (Split-Path $Path) }

@niheaven
Copy link
Member Author

@Ash258 I've found it and fixed in this PR, it should be okay when this is merged. 😄

It "accept array path param" -Skip:$isUnix {
$to = test_extract "Expand-InnoArchive" @($test)
$to | Should -Exist
"$to\empty" | Should -Exist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incosistency. You are using Join-Path in #L74

Expand-7zipArchive $Path $DestinationPath -Removal
return
process {
if (!$DestinationPath) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this in process block?

# Remove original archive file
Remove-Item $currentPath -Force
}
}
}
}

function Expand-ZipArchive {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add synopsis to functions

@@ -39,12 +39,13 @@ function Expand-7zipArchive {
[CmdletBinding()]
param (
[Parameter(Mandatory = $true, Position = 0, ValueFromPipeline = $true)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is not ValueFromPipelineByPropertyName?

You just limit me with usage of custom object.

A

Comment on lines +99 to +103
switch ($Overwrite) {
"All" { $ArgList += "-aoa" }
"Skip" { $ArgList += "-aos" }
"Rename" { $ArgList += "-aou" }
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use single quotes

}
$currentDestinationPath = $currentDestinationPath.TrimEnd("\")
if ($currentExtractDir) {
$OriDestinationPath = $currentDestinationPath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

local variable

Suggested change
$OriDestinationPath = $currentDestinationPath
$oriDestinationPath = $currentDestinationPath

Comment on lines +254 to +257
"^[^{].*" { $ArgList += "-c{app}\$currentExtractDir" }
"^{.*" { $ArgList += "-c$currentExtractDir" }
Default { $ArgList += "-c{app}" }
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • single quotes
  • default is lowercased when switch is lowercased

$currentExtractDir = $null
}
if ($currentExtractDir) {
$OriDestinationPath = $currentDestinationPath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

local varaible

# try to fall back to 7zip if path is too long
if (Test-HelperInstalled -Helper 7zip) {
Expand-7zipArchive $currentPath $currentDestinationPath -Removal
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you break whole foreach?

@niheaven
Copy link
Member Author

@Ash258 I would like to rebase this PR onto #3502 and check the whole code again after #3502 is merged. There're many merging conflicts between these two PRs...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants