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 missing parameter aliases to align with caller/callee #1522

Merged
merged 3 commits into from
Oct 12, 2023

Conversation

kyllath
Copy link
Contributor

@kyllath kyllath commented Oct 11, 2023

Fix issue where none of 'HeaderRow', 'TopRow' and 'StartRow' work in ConvertFrom-ExcelToSQLInsert, preventing you from skipping rows.

Issue is that aliases are missing from the parameter list of ConvertFrom-ExcelData.
ConvertFrom-ExcelToSQLInsert accepts 'HeaderRow', 'TopRow' and 'StartRow', but passes 'StartRow', while ConvertFrom-ExcelData only accepts 'HeaderRow'

FIX: Alter ConvertFrom-ExcelData to have the same aliases for the parameter as both its caller, ConvertFrom-ExcelToSQLInsert and its callee, Import-Excel. All three now have:
[Alias('HeaderRow', 'TopRow')]
[Int]$StartRow = 1,

@dfinke
Copy link
Owner

dfinke commented Oct 12, 2023

Hmm. Feels like TopRow should be added as a param not an alias. It's been a long time since I've benn in this part of the code.

@kyllath
Copy link
Contributor Author

kyllath commented Oct 12, 2023

The reason I chose this approach is that ConvertFrom-ExcelToSQLInsert always calls ConvertFrom-ExcelData, which in turn calls Import-Excel. Both ConvertFrom-ExcelToSQLInsert and Import-Excel have StartRow as the param with HeaderRow and TopRow as aliases, so this way just makes all three consistent with the minimum of changes.

@dfinke dfinke self-assigned this Oct 12, 2023
@dfinke dfinke added the bug label Oct 12, 2023
@dfinke dfinke merged commit 6174401 into dfinke:master Oct 12, 2023
9 checks passed
@dfinke
Copy link
Owner

dfinke commented Oct 12, 2023

@kyllath Thank you for the update!

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

Successfully merging this pull request may close these issues.

2 participants