Small cleanup Invoke-RestMethod#19490
Conversation
...erShell.Commands.Utility/commands/utility/WebCmdlet/Common/InvokeRestMethodCommand.Common.cs
Outdated
Show resolved
Hide resolved
|
|
||
| // NOTE: Tests use this verbose output to verify the encoding. | ||
| WriteVerbose(string.Create(System.Globalization.CultureInfo.InvariantCulture, $"Content encoding: {encodingVerboseName}")); | ||
| WriteVerbose(string.Create(System.Globalization.CultureInfo.InvariantCulture, $"Content encoding: {encoding.HeaderName}")); |
There was a problem hiding this comment.
I don't think it will throw
Test:
[System.Text.Encoding]::GetEncodings().GetEncoding().Count -eq `
([System.Text.Encoding]::GetEncodings().GetEncoding() | % {$_.EncodingName}).Count -eq `
([System.Text.Encoding]::GetEncodings().GetEncoding() | % {$_.HeaderName}).Count -eq 116 #tested on windowsThere was a problem hiding this comment.
There is throw in implementation.
There was a problem hiding this comment.
EncodingName: https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Text/Encoding.cs,355
HeaderName: https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Text/Encoding.cs,371
Both can throw in the same condition: https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Text/Encoding.cs,327
So I don't think the try-catch is correct
There was a problem hiding this comment.
I don't understand - you point NotSupportedException in the implementation but says the try-catch is not correct.
There was a problem hiding this comment.
I'll try to explain better.
-
We obtain the encoding from
StreamHelper.DecodeStream()-->TryGetEncoding()--> this will always generate a valid encoding, if there is an unrecognized encoding it will be replaced byContentHelper.GetDefaultEncoding()--> UTF8 -
Every valid encoding (all 116 of them) has both
encoding.EncodingNameandencoding.HeaderNameso we don't need to checkstring.IsNullOrEmpty(encoding.HeaderName)and we can simply choose the name we like best -
If an invalid encoding got to the
encodingVerboseNametry-catch:
string encodingVerboseName;
try
{
encodingVerboseName = string.IsNullOrEmpty(encoding.HeaderName) ? encoding.EncodingName : encoding.HeaderName;
}
catch (NotSupportedException)
{
encodingVerboseName = encoding.EncodingName;
}both the names throw in the same condition (EncodingTable.GetCodePageDataItem(_codePage) is null) so both would throw:
Cases:
string.IsNullOrEmpty(encoding.HeaderName) --> encodingVerboseName = encoding.EncodingName --> throw --> catch --> encodingVerboseName = encoding.EncodingName --> throw
!string.IsNullOrEmpty(encoding.HeaderName) --> encodingVerboseName = encoding.HeaderName --> throw --> catch --> encodingVerboseName = encoding.EncodingName --> throw
There was a problem hiding this comment.
3 is good catch and we could use String.Empty.
I cannot agree with 1. and 2. points since it is not safe as the previous code can be changed and also there is dynamic code pages registration.
There was a problem hiding this comment.
I updated the code following your suggestions
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
🎉 Handy links: |
PR Summary
Small cleanup Invoke-RestMethod
PR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).