Update Environment.OSVersion for browser platform #38790
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
| @@ -3072,6 +3072,7 @@ public enum PlatformID | |||
| Unix = 4, | |||
| Xbox = 5, | |||
| MacOSX = 6, | |||
| Unknown = 7, | |||
There was a problem hiding this comment.
Not yet. Does it make sense to you?
stephentoub
added
the
NO-MERGE
|
Blocked by #38984 |
marek-safar
removed
the
NO-MERGE
| @@ -3072,6 +3072,7 @@ public enum PlatformID | |||
| Unix = 4, | |||
| Xbox = 5, | |||
| MacOSX = 6, | |||
| Other = 7, | |||
There was a problem hiding this comment.
Not related to this PR, but, @terrajobst, I see that the src has EditorBrowsableState.Never on some entries, but the ref doesn't have that. Which is correct?
There was a problem hiding this comment.
It looks like they're supposed to be hidden in the ref:
#21660
dotnet/corefx#19697
It looks like they were removed as part of:
https://github.com/dotnet/runtime/pull/31991/files#diff-1d02aa10f6540e37197ff9f090ca1d9eR2944
https://github.com/dotnet/runtime/pull/31991/files#diff-5450da078ed8d6b7e0e7413e10ab68c6L953
@jkotas, was that intentional?
There was a problem hiding this comment.
It was not intentional. I guess I have just hit a GenAPI bug that I did not notice.
| { | ||
| private static OperatingSystem GetOSVersion() | ||
| { | ||
| return new OperatingSystem(PlatformID.Other, new Version(1, 0, 0, 0)); |
There was a problem hiding this comment.
Can you add a test for this?
There was a problem hiding this comment.
There are tests already which are being fixed/enabled as part of #38996
|
Reopening - it was closed by other PR |
Creating an OperatingSystem with PlatformID.Other would result in an exception about an unexpected enum value. dotnet#38790 was missing some tests that verified this behavior.
Creating an OperatingSystem with PlatformID.Other would result in an exception about an unexpected enum value. #38790 was missing some tests that verified this behavior.
ghost
locked as resolved and limited conversation to collaborators
Introduce PlatformID.Unknown for this is "legacy" API to allow returning some sensible value
Gathering quick feedback if this makes sense instead of e.g. PNSE
@lewing