Refactor UTF-7 encoding detection #18384

xtqqczze · 2022-10-25T22:52:03Z

Motivation

Remove the unnecessary string comparison in EncodingConversion.Convert.

PowerShell/src/System.Management.Automation/utils/EncodingUtils.cs

Line 67 in f3f56d4

if (string.Equals(encoding, Utf7, StringComparison.OrdinalIgnoreCase))

Context

cc: @JamesWTruher

xtqqczze · 2022-10-26T00:22:43Z

Perhaps @JamesWTruher could review?

src/System.Management.Automation/namespaces/FileSystemProvider.cs

ghost · 2022-11-05T14:01:03Z

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

xtqqczze · 2022-11-05T21:50:04Z

PowerShell-CI-macos failures seem unrelated:

dotnet build failed with error: Expected binary was not created:
     | bin/Debug/net7.0/publish/Microsoft.PowerShell.NamedPipeConnection.dll

daxian-dbw · 2022-11-07T18:15:45Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+                void WarnIfUtf7Encoding(Encoding value)
+                {
+                    const int CodePageUtf7 = 65000;
+
+                    // Use the recommended pattern to check for UTF-7.
+                    // https://docs.microsoft.com/dotnet/core/compatibility/core-libraries/5.0/utf-7-code-paths-obsolete
+                    if (value is { CodePage: CodePageUtf7 })
+                    {
+                        _provider.WriteWarning(PathUtilsStrings.Utf7EncodingObsolete);
+                    }
+                }


This seems overkill to me. I prefer the original code.

@xtqqczze Can you please respond to this comment? Thanks!

I prefer the symbolic constant because it explains the meaning at a glance without having to read the comment for an explanation.

@iSazonov, @SteveL-MSFT Thoughts?

I guess @daxian-dbw don't like the modern syntax in the place but ok with the new name WarnIfUtf7Encoding. If so I agree with him.

I should be clearer on my comment.
It's not the syntax value is { CodePage: CodePageUtf7 }, but that you have a local method in a setter. Please avoid having a local method in a setter unless it's really needed.

@xtqqczze Please revert the change.

@xtqqczze - please respond to this feedback

pull-request-quantifier-deprecated · 2022-11-14T22:54:10Z

This PR has 31 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       : +17 -14
Percentile : 12.4%

Total files changed: 9

Change summary by file extension:
.cs : +17 -14

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 detected.
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.

ghost · 2022-11-30T02:01:09Z

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

xtqqczze added 4 commits October 25, 2022 23:40

Use WarnIfObsolete in EncodingConversion

ffe4b12

Retarget documentation link

e534ca6

Extract to WarnIfObsolete

f3b2fd4

Refactor WarnIfObsolete

76c3d3b

pull-request-quantifier-deprecated bot added the Extra Small label Oct 25, 2022

ghost assigned adityapatwardhan Oct 25, 2022

xtqqczze marked this pull request as ready for review October 26, 2022 00:19

xtqqczze requested a review from anmenaga as a code owner October 26, 2022 00:19

Reword comments

b1624fd

iSazonov reviewed Oct 28, 2022

View reviewed changes

src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated Show resolved Hide resolved

ghost added the Review - Needed The PR is being reviewed label Nov 5, 2022

Rename WarnIfObsolete

8441e0f

xtqqczze requested a review from PaulHigin as a code owner November 5, 2022 20:38

iSazonov approved these changes Nov 6, 2022

View reviewed changes

daxian-dbw requested changes Nov 7, 2022

View reviewed changes

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels Nov 7, 2022

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 14, 2022

xtqqczze closed this Nov 14, 2022

xtqqczze reopened this Nov 14, 2022

ghost added the Review - Needed The PR is being reviewed label Nov 30, 2022

daxian-dbw added CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log Needs-Triage The issue is new and needs to be triaged by a work group. labels May 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor UTF-7 encoding detection #18384

Refactor UTF-7 encoding detection #18384

xtqqczze commented Oct 25, 2022 •

edited

xtqqczze commented Oct 26, 2022

ghost commented Nov 5, 2022

xtqqczze commented Nov 5, 2022

daxian-dbw Nov 7, 2022

daxian-dbw Nov 14, 2022

xtqqczze Nov 14, 2022 •

edited

xtqqczze Nov 21, 2022

iSazonov Nov 22, 2022 •

edited

daxian-dbw Nov 22, 2022 •

edited

iSazonov Dec 14, 2022

adityapatwardhan Jan 3, 2023

pull-request-quantifier-deprecated bot commented Nov 14, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

ghost commented Nov 30, 2022

Refactor UTF-7 encoding detection #18384

Are you sure you want to change the base?

Refactor UTF-7 encoding detection #18384

Conversation

xtqqczze commented Oct 25, 2022 • edited

Motivation

Context

xtqqczze commented Oct 26, 2022

ghost commented Nov 5, 2022

xtqqczze commented Nov 5, 2022

daxian-dbw Nov 7, 2022

Choose a reason for hiding this comment

daxian-dbw Nov 14, 2022

Choose a reason for hiding this comment

xtqqczze Nov 14, 2022 • edited

Choose a reason for hiding this comment

xtqqczze Nov 21, 2022

Choose a reason for hiding this comment

iSazonov Nov 22, 2022 • edited

Choose a reason for hiding this comment

daxian-dbw Nov 22, 2022 • edited

Choose a reason for hiding this comment

iSazonov Dec 14, 2022

Choose a reason for hiding this comment

adityapatwardhan Jan 3, 2023

Choose a reason for hiding this comment

pull-request-quantifier-deprecated bot commented Nov 14, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

ghost commented Nov 30, 2022

xtqqczze commented Oct 25, 2022 •

edited

xtqqczze Nov 14, 2022 •

edited

iSazonov Nov 22, 2022 •

edited

daxian-dbw Nov 22, 2022 •

edited