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

Add ParameterSet "Now-Table" and making ParameterSets clearer #580

Closed
wants to merge 4 commits into from

Conversation

ili101
Copy link
Contributor

@ili101 ili101 commented Apr 11, 2019

Before we had the default parameter set "Default" that meant if there where 0 parameters it's "Now" and if there is more it's "Path".
After the update "Default" meant "Now" always except when "Path" is set, making "New" the DefaultParameterSet in practice but not actually the DefaultParameterSetName in code.

That can be a little confusing for example in Show-Command if the user will select "Default" but will get different behaviors if he fills -Path or not ("New" behavior if not and "Default" behavior if -Path used)
2019-04-11_12-35-48

Also parameter set "New-Table" was missing so you can do Export-Excel -TableName 'Table' to get -Now behavior but if you explicitly do the same thing Export-Excel -TableName 'Table' -Now you will get an error that "Parameter set cannot be resolved"

So I made the "Now" parameter set set the default, this should not change anything but make it more clear if you are using "Now" or not in Show-Command
Also I added the missing "New-Table" parameter set.
2019-04-11_12-26-42

While Writing this I found a problem:
$DataTable | Export-Excel -TableName 'Table' is not working, it returns "Supply values for the following parameters: ExcelPackage:" So I workaround it, I reported this to the PowerShell team last year but there is no response yet PowerShell/PowerShell#8516

@dfinke
Copy link
Owner

dfinke commented Apr 11, 2019

@jhoneill review?

I'm planning to publish v6 tomorrow, Friday. No rush on this, prefer to wait.

Does the $DataTable | Export-Excel -TableName 'Table' work in pwsh? I recently had conversations with some about other parameter set/binding issues.

@ili101
Copy link
Contributor Author

ili101 commented Apr 11, 2019

Same Problem on 6.2

PS C:\Program Files\PowerShell\6> $PSVersionTable.PSVersion

Major  Minor  Patch  PreReleaseLabel BuildLabel
-----  -----  -----  --------------- ----------
6      2      0

PS C:\Program Files\PowerShell\6> 5 | Export-Excel -TableName 'Table'

cmdlet Export-Excel at command pipeline position 1
Supply values for the following parameters:
ExcelPackage:

@jhoneill
Copy link
Contributor

jhoneill commented Apr 12, 2019

  1. You can not combine -tableName with -AutoNameRange. So we initially had 2 parameter sets one for each. Then when I added -Excel package that became four sets (2 path, 2 Package). -Now turns on autosize, show, and orgianlly always turned on autoNameRange.

  2. One recent change is that now is implied whenever there is neither Package, or Path. So that this works with -tablename, autoNameRange is only selected if -tableName is not. What I should have done is gone back and made two parameter sets for -Now (now-Table and now-default) that looks to be what @ili101 has done. ... if it is, then yes, approve ...

  3. When table is specified on its own, PowerShell knows it isn't in the default parameter set and it has to be one of table and package table. I don't know how it chooses whether to ask for a path or a package. However specifying table and none of path/package/now has never been supported. Until the change in (2)x | export-excel -tablename y would have crashed losing the input data when it tried to save the file.
    .

Get-SQL -Connection DSN=LR
Get-SQL  -sql "SELECT name AS CollectionName FROM AgLibraryCollection Collection ORDER BY CollectionName" -OutputVariable table

followed by either
$table | export-excel -tablename lr -now
or
export-excel -tablename lr -now -InputObject $table
both work on 6.2 without now (or path, or package) you'll be prompted for a parameter

@ili101
Copy link
Contributor Author

ili101 commented Apr 12, 2019

I don't know how it chooses whether to ask for a path or a package.

I also tried to figure it out but failed, I hope that they will add the option to provide an array of "Default" parameter sets to be evaluated in the provided order, and not just one DefaultParameterSetName. that will make situations like this much simpler.

Edit. Looks like that when the pipeline is used it always go to the set with the mandatory parameter, which is the less preferable option usually as it's the opposite of what it dose when the pipeline is not used.

function Test-It
{
    Param(
        [Parameter(ParameterSetName = 'A')]
        $A,
        [Parameter(ParameterSetName = 'B', Mandatory)]
        $B,
        [Parameter(ValueFromPipeline = $true)]
        $InputObject
    )
    $PSCmdlet.ParameterSetName
}

# This will select set A automatically.
Test-It -InputObject 0
# This will select set B and ask you for input.
0 | Test-It

Adding the ValueFromPipelineByPropertyName to the Mandatory parameter "Fixes it"

function Test-It
{
    Param(
        [Parameter(ParameterSetName = 'A')]
        $A,
        [Parameter(ParameterSetName = 'B', Mandatory, ValueFromPipelineByPropertyName)]
        $B,
        [Parameter(ValueFromPipeline = $true)]
        $InputObject
    )
    $PSCmdlet.ParameterSetName
}

# This will select set A automatically.
Test-It -InputObject 0
# This will select set A automatically.
0 | Test-It

So this is essentially what I did.

I digged in the commits and saw that you @jhoneill was the one that come up with the ValueFromPipelineByPropertyName fix originally 75676a8 how did you figured it out? I didn't see it used anywhere else.

@ili101
Copy link
Contributor Author

ili101 commented Apr 13, 2019

I just want to mention that non of the problems addressed in this PR are braking the functionality of the module, this is just to make the module more user friendly and behave consistently mostly for new users.

@jhoneill
Copy link
Contributor

@ili101 see https://jamesone111.wordpress.com/2016/11/30/powershell-piped-parameter-peculiarities-and-a-palliative-pattern/ for background on using ValueFromPipelineByPropertyName

As that says ... it seems that when you are not piping anything, PowerShell is happy to decide parameter sets based on the absence of a mandatory parameter. When you are piping it stops doing that, but ValueFromPipelineByPropertyName seems to put it back. I don't properly understand it, but I've found it fixes some cases.

@jhoneill
Copy link
Contributor

Now I have had a chance to look at this more I'm finding some problems.
(1). If what is piped in has a path property (which is very possible) then ValueFromPipelineByPropertyName may cause problems
(2) It works if Table name is specified but not if TableStyle is specified without a name. This is a worse failure because it generates an error for each item instead of one error. In the present version table style with no name has no effect.

So I'd stick to just a parameter set to allow now and tables.

@dfinke Your call, but we could solve this by just getting rid of the parameter sets for table vs AutoFilter, and then have
if table
elseif Autofilter
If someone specifies table they get a table with filtering whether they specify autofiler or not ... and if you change from "I want a filter" to "Wouldn't my filtered data look prettier as a table" you don't need to remove the autofilter parameter. If we do that, I'd also fix it to use a random table name if style is specified without a name.

@ili101
Copy link
Contributor Author

ili101 commented Apr 14, 2019

Pushed a quick test removing the Table ParameterSets and all the ValueFromPipelineByPropertyName, could be good.
Also if you do $ws.Tables.Add($Range, $null) or $ws.Tables.Add($Range, '') EPPlus will create "Table1" ,"Table2", "Table3"... but I don't know if it is documented and safe to rely on?

@ili101
Copy link
Contributor Author

ili101 commented May 2, 2019

Was implemented in #590 👍

@ili101 ili101 closed this May 2, 2019
@dfinke
Copy link
Owner

dfinke commented May 2, 2019

Thanks for closing.

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