GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||
GeezerVison is showing github.com — Enable more nullable annotations in WebCmdlets by CarloToso · Pull Request #19359 · PowerShell/PowerShell Conversation
|
| bool result = false; | ||
| try | ||
| { | ||
| ArgumentException.ThrowIfNullOrEmpty(characterSet); |
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think it is wrong. Encoding.GetEncoding(null) returns OS default but Encoding.GetEncoding(string.Empty) throw. We should investigate the last case and if it is a bug we should fix in another PR.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think the desired behaviour is that both cases (characterSet == null; characterSet == string.Empty) should should return result = false and encoding = UTF8 (the default). Do you agree?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The code is in try-catch. So it makes no sense. Please remove.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I know it doesn't make sense, I added it to avoid a possible null reference error for characterSet, could you point me to a better solution?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Null reference error? In TryGetEncoding? It is impossible since Encoding.GetEncoding(null) returns OS default, for string.Empty try-catch will catch and set default too. Also it will set default if .Net doesn't know a value in characterSet.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The warning before the last 2 commits was Possible null reference argument for parameter 'name' in 'Encoding Encoding.GetEncoding(string name)'., and the build fails on warnings
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Since [System.Text.Encoding]::GetEncoding($null) return OS default without exception I suggest to open new issue in .Net.
Today we could use a workaround with ! operator and comment with reference to the new .Net issue.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I looked a bit more into it
Test:
$source = @" using System; using System.Text; public class EncodingTest { public static Encoding GetEncoding1(string name) { return Encoding.GetEncoding(name); } public static Encoding GetEncoding2(int name) { return Encoding.GetEncoding(name); } } "@ Add-Type -TypeDefinition $source -Language CSharp -ReferencedAssemblies netstandard, System.Text.Encoding [EncodingTest]::GetEncoding1($null) #--> ERROR [EncodingTest]::GetEncoding2($null) #--> windows-1252 [EncodingTest]::GetEncoding1("") #--> ERROR [EncodingTest]::GetEncoding2("") #--> windows-1252 [EncodingTest]::GetEncoding2(0) #--> windows-1252
In our case we are using Encoding.GetEncoding(string name) so it throws an error with null or empty that gets caught, we don't have a bug everything works as planned. Unfortunalely Encoding.GetEncoding(string name) with null gives us a warning, as you said we could use ! operator.
If Encoding.GetEncoding(string name) with name == null returned windows-1252 that wouldn't be a desired behaviour for us in this case, because we want to default to UTF-8
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Unrelated error |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Ilya (@iSazonov) can you merge this PR? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
CarloToso Please look test fails. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Ilya (@iSazonov) all tests pass |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
🎉 Handy links: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.