Skip to content

Tee-Object: add -Encoding parameter #14539

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.

Sign up for GitHub

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

Jump to bottom
Merged
daxian-dbw merged 20 commits into from
Nov 2, 2021
Merged

Tee-Object: add -Encoding parameter #14539

daxian-dbw merged 20 commits into from
Nov 2, 2021

Conversation

@rpolley
Copy link
Contributor

@rpolley rpolley commented Jan 5, 2021 •
edited by iSazonov
Loading

PR Summary

Fix #11104.

Adds a new -Encoding parameter to Tee-Object that controls what text encoding to use for the output file. If not provided, the cmdlet behaves as before and uses the default encoding.

PR Context

Most cmdlets that deal with text file IO have an -Encoding parameter that tells the cmdlet which encoding to use when reading/writing the file. However, Tee-Object does not. This pull request adds this functionality. Closes #11104.

PR Checklist

@ghost ghost assigned daxian-dbw Jan 5, 2021
@ghost
Copy link

ghost commented Jan 5, 2021 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@rpolley rpolley changed the title Tee-Object: add -Encoding parameter WIP: Tee-Object: add -Encoding parameter Jan 5, 2021
rpolley added 2 commits January 4, 2021 19:42
@rpolley
Merge branch 'Add--Encoding-parameter-for-Tee-Object' of github.com:r…
2a7bff3 
…polley/PowerShell into Add--Encoding-parameter-for-Tee-Object
@rpolley
Merge branch 'Add--Encoding-parameter-for-Tee-Object' of github.com:r…
6adce0d 
…polley/PowerShell into Add--Encoding-parameter-for-Tee-Object
@rpolley rpolley changed the title WIP: Tee-Object: add -Encoding parameter Tee-Object: add -Encoding parameter Jan 5, 2021
iSazonov
iSazonov reviewed Jan 5, 2021
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix CIs.

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jan 5, 2021
iSazonov
iSazonov reviewed Jan 7, 2021
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpolley Please look test fails.

@iSazonov iSazonov requested a review from SteveL-MSFT January 7, 2021 11:50
iSazonov
iSazonov reviewed Jan 8, 2021
View reviewed changes
src/Microsoft.PowerShell.Commands.Utility/commands/utility/Tee-Object.cs Outdated
/// <summary>
/// Sets the encoding to use when writing the file.
/// </summary>
[Parameter(Mandatory = false)]
Copy link
Collaborator

@iSazonov iSazonov Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert - I was wrong.

@rpolley
Delete \
563d96c 
Looks like this was some sort of temp file created during merge commit that ended up in the branch somehow
@rpolley
Remove mandatory = false from parameter annotation
b303c2f
iSazonov
iSazonov reviewed Jan 13, 2021
View reviewed changes
src/Microsoft.PowerShell.Commands.Utility/commands/utility/Tee-Object.cs Outdated
}

// passthrough Encoding parameter
_commandWrapper.AddNamedParameter("Encoding", Encoding);
Copy link
Collaborator

@iSazonov iSazonov Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work for Tee-Object -Variable because we assign Encoding parameter to Set-Variable cmdlet. Please see previous

PowerShell/src/Microsoft.PowerShell.Commands.Utility/commands/utility/Tee-Object.cs

Lines 117 to 121 in b303c2f

// variable parameter set
_commandWrapper.Initialize(Context, "set-variable", typeof(SetVariableCommand));
_commandWrapper.AddNamedParameter("name", _variable);
// Can't use set-var's passthru because it writes the var object to the pipeline, we want just
// the values to be written

Copy link
Contributor Author

@rpolley rpolley Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't even notice this, thanks. I updated it so that the Encoding parameter is only used for the File and LiteralFile parameter sets.

iSazonov
iSazonov reviewed Jan 14, 2021
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/Tee-Object.Tests.ps1 Outdated
) {
param($Encoding)
$teefile = $testfile
Write-Output -InputObject teeobjecttest3 | Tee-Object -FilePath $teefile -Encoding $Encoding
Copy link
Collaborator

@iSazonov iSazonov Jan 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really the test doesn't check encoding because write ASCII only.
Please see Verifies -Tail with content that uses an explicit/implicit encoding from Get-Content.Tests.ps1 . We could use the same string with native chars instead of teeobjecttest3.

@iSazonov
Copy link
Collaborator

iSazonov commented Jan 20, 2021

@rpolley Please look CodeFactor and CI issues.

@ghost ghost added the Review - Needed The PR is being reviewed label Jan 28, 2021
@ghost
Copy link

ghost commented Jan 28, 2021

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@pull-request-quantifier-deprecated
Copy link

pull-request-quantifier-deprecated bot commented Nov 2, 2021

This PR has 30 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details 

Label      : Extra Small
Size       : +23 -7
Percentile : 12%

Total files changed: 1

Change summary by file extension:
.ps1 : +23 -7

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

  • Fast and predictable releases to production:
    • Optimal size changes are more likely to be reviewed faster with fewer
      iterations.
    • Similarity in low PR complexity drives similar review times.
  • Review quality is likely higher as complexity is lower:
    • Bugs are more likely to be detected.
    • Code inconsistencies are more likely to be detetcted.
  • Knowledge sharing is improved within the participants:
    • Small portions can be assimilated better.
  • Better engineering practices are exercised:
    • Solving big problems by dividing them in well contained, smaller problems.
    • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

  • Use the PullRequestQuantifier to quantify your PR accurately
    • Create a context profile for your repo using the context generator
    • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
    • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
    • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
  • Change your engineering behaviors
    • For PRs that fall outside of the desired spectrum, review the details and check if:
      • Your PR could be split in smaller, self-contained PRs instead
      • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

  • One line was added: +1 -0
  • One line was deleted: +0 -1
  • One line was modified: +1 -1 (git diff doesn't know about modified, it will
    interpret that line like one addition plus one deletion)
  • Change percentiles: Change characteristics (addition, deletion, modification)
    of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

@daxian-dbw daxian-dbw merged commit cf22188 into PowerShell:master Nov 2, 2021
@ghost ghost removed the Review - Needed The PR is being reviewed label Nov 2, 2021
@iSazonov iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Nov 2, 2021
@iSazonov
Copy link
Collaborator

iSazonov commented Nov 2, 2021 •
edited
Loading

We need to open doc issue.
/cc @sdwheeler

@sdwheeler
Copy link
Collaborator

sdwheeler commented Nov 2, 2021

@iSazonov This was documented in MicrosoftDocs/PowerShell-Docs#7902

@iSazonov iSazonov removed the Documentation Needed in this repo Documentation is needed in this repo label Nov 2, 2021
@ghost
Copy link

ghost commented Dec 16, 2021

🎉v7.3.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Dec 16, 2021
Closed
TrapGodBrim pushed a commit to TrapGodBrim/PowerShell that referenced this pull request Jan 19, 2022
@rpolley @TrapGodBrim
Add more tests to Tee-Object -Encoding (PowerShell#14539)
84f3e21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

Assignees

@daxian-dbw daxian-dbw

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Extra Small

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Tee-Object should have an -Encoding parameter

4 participants

@rpolley @iSazonov @sdwheeler @daxian-dbw