Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Remove -Force check in ReconcilePreexistingPropertyNames method in Export-Csv.#6796

Closed
sethvs wants to merge 1 commit into
PowerShell:masterPowerShell/PowerShell:masterfrom
sethvs:ExportCsvsethvs/PowerShell:ExportCsvCopy head branch name to clipboard
Closed

Remove -Force check in ReconcilePreexistingPropertyNames method in Export-Csv.#6796
sethvs wants to merge 1 commit into
PowerShell:masterPowerShell/PowerShell:masterfrom
sethvs:ExportCsvsethvs/PowerShell:ExportCsvCopy head branch name to clipboard

Conversation

@sethvs

@sethvs sethvs commented May 2, 2018

Copy link
Copy Markdown
Contributor

PR Summary

Now, when you use Export-Csv with -Append parameter and new properties don't exist in existing file:

  • -Force:$false: error is thrown
  • -Force:$true: cmdlet silently skips adding new properties.

Should we remove -Force check in ReconcilePreexistingPropertyNames method, so that error is thrown in both cases - when -Force:$true and -Force:$false.

Or is it better to make cmdlet overwrite existing file with new data if -Force:$true?

PR Checklist

@sethvs

sethvs commented May 2, 2018

Copy link
Copy Markdown
Contributor Author

Can you please restart tests?

@iSazonov

iSazonov commented May 2, 2018

Copy link
Copy Markdown
Collaborator

I restarted CI Travis MacOs tests.

@iSazonov iSazonov requested a review from SteveL-MSFT May 2, 2018 13:41
@iSazonov

iSazonov commented May 2, 2018

Copy link
Copy Markdown
Collaborator

@mklement0 Could you please review the change too?

@iSazonov iSazonov self-assigned this May 2, 2018
@mklement0

mklement0 commented May 2, 2018

Copy link
Copy Markdown
Contributor

My vote is for not making this change, which is certainly a breaking one:

While the docs don't really say what -Force is for - which should definitely be added - de facto it has served to force addition of objects even if they have unrelated / additional lack properties that map onto existing columns, which is a useful option to have if applied intentionally (that's where the docs come in).

Note that the error message you currently get with incompatible missing properties clearly states that -Force can be used to force appending.

@sethvs:

Please make it clearer up front when your changes will be breaking, and ideally create an issue before starting a PR, ideally by identifying the "bucket" that the change falls into.

Also, sometimes there's an alternative to a breaking change: in #6797 you tried to simply rename a parameter, but the better course of action is to add an alias.

@mklement0

mklement0 commented May 2, 2018

Copy link
Copy Markdown
Contributor

I've opened a docs issue to request documentation of the current behavior: MicrosoftDocs/PowerShell-Docs#2392

@sethvs

sethvs commented May 2, 2018

Copy link
Copy Markdown
Contributor Author

@mklement0 Agree about -Force.
This is what discussion for.
Closing.

@sethvs sethvs closed this May 2, 2018
@mklement0

Copy link
Copy Markdown
Contributor

@sethvs:

Glad to hear it.

The reason I was suggesting creating an issue before submitting a PR is that the issue allows a broader discussion as to whether and how to implement a change / feature first.

I know that this PR was simple, but generally you run the risk of wasted effort without having consensus on what should be done first.

@sethvs sethvs deleted the ExportCsv branch May 3, 2018 09:45
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.