Skip to content

Conversation

@sethvs
Copy link
Contributor

@sethvs sethvs commented May 2, 2018

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
Copy link
Contributor Author

sethvs commented May 2, 2018

Can you please restart tests?

@iSazonov
Copy link
Collaborator

iSazonov commented May 2, 2018

I restarted CI Travis MacOs tests.

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

iSazonov commented May 2, 2018

@mklement0 Could you please review the change too?

@iSazonov iSazonov self-assigned this May 2, 2018
@mklement0
Copy link
Contributor

mklement0 commented May 2, 2018

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
Copy link
Contributor

mklement0 commented May 2, 2018

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

@sethvs
Copy link
Contributor Author

sethvs commented May 2, 2018

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

@sethvs sethvs closed this May 2, 2018
@mklement0
Copy link
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