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

Fix import excel headers #713

Merged
merged 3 commits into from
Nov 10, 2019
Merged

Fix import excel headers #713

merged 3 commits into from
Nov 10, 2019

Conversation

dfinke
Copy link
Owner

@dfinke dfinke commented Nov 4, 2019

No description provided.

@dfinke
Copy link
Owner Author

dfinke commented Nov 4, 2019

#712

@jhoneill, before I go down the rabbit hole. Looks like checking for HeadingName and using $Rows = $StartRow..$EndRow solves it.

Put some basic testing in place to check offsets.

Thoughts? Before I go deeper.

@jhoneill
Copy link
Contributor

jhoneill commented Nov 5, 2019

I think this is by design - not that the design was right

Basically we have two choices. "There are no headers" and "Replace the headers with ...".
If I understand what this change does, anything which has headers which need to be replaced will get them as a row of data. Really if we are going to fix the "I have no headers , use these names as headers" we need to enable both the options together.

@ili101
Copy link
Contributor

ili101 commented Nov 5, 2019

If you already digging in this subject can you see if you can implement the features I suggested in #579

I was thinking maybe doing something like this to address this problem + #579 + more option we will want to add now or later:

# This is an incomplete mockup
# SYNTAX: New-HeaderDefinition [-MyTableHasHeaders] [-IncludeUndefinedHeaders]
$Headers = New-HeaderDefinition -MyTableHasHeaders
# SYNTAX set OriginalIndex : Add-HeaderDefinitionColumn -OriginalIndex <int> -NewName <string> [-NewIndex <int>]
# SYNTAX set OriginalName  : Add-HeaderDefinitionColumn -OriginalName <string> -NewName <string> [-NewIndex <int>]
$Headers | Add-HeaderDefinitionColumn -OriginalIndex 1 -NewName 'New Name'
$Headers | Add-HeaderDefinitionColumn -OriginalName 'C3' -NewName 'Was C3' -NewIndex 1

Import-Excel -Headers $Headers

<#
Will replace
| C1 | C2 | C3 |
| A  | B  | C  |
with
| Was C3 | New Name |
| C      | A        |
#>

This way we can keep the number of parameters on Import-Excel to the minimum while giving more flexibility, but it requires more then one line of code to use.
What do you think? good or overkill?

@dfinke
Copy link
Owner Author

dfinke commented Nov 7, 2019

I'm all for doing the simplest change to get the desired outcome of the initial bug report, with an eye towards no breaking changes.

Does the code change in the PR satisfy that?

@jhoneill
Copy link
Contributor

jhoneill commented Nov 8, 2019

I think the simplest way is to change this bit at line 345 in importexcel.psm1

                if ($NoHeader) {
                    $i = 0
                    foreach ($C in $Columns) {
                        $i++
                        $C | Select-Object @{N = 'Column'; E = { $_ } }, @{N = 'Value'; E = { 'P' + $i } }
                    }
                }
                elseif ($HeaderName) {
                    $i = 0
                    foreach ($H in $HeaderName) {
                        $H | Select-Object @{N = 'Column'; E = { $Columns[$i] } }, @{N = 'Value'; E = { $H } }
                        $i++
                    }
                }

Put if($headerName) first and then elseif ($Noheader). That will set the names then it can either be run with startrow 2 (or top of data +1 ) or with -NoHeader (which means simplifying the parameter sets. Update help & tests, and job done.

I

@dfinke
Copy link
Owner Author

dfinke commented Nov 8, 2019

Doing it KISS. @jhoneill Looks like the -DataOnly still works on skipping empty rows, but not empty column. I'll check it out, once done, I'll merge.

Any thoughts on the tests etc.? I didn't touch the parameter sets. I think the help looks good.

@dfinke dfinke merged commit 5959a97 into master Nov 10, 2019
@dfinke dfinke deleted the FixImportExcelHeaders branch November 16, 2019 15:26
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