Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix Environment.OSVersion.Platform output on MacOSX#19697

Merged
stephentoub merged 1 commit intodotnet:masterfrom
ViktorHofer:EnvironmentPlatformID
May 13, 2017
Merged

Fix Environment.OSVersion.Platform output on MacOSX#19697
stephentoub merged 1 commit intodotnet:masterfrom
ViktorHofer:EnvironmentPlatformID

Conversation

@ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented May 12, 2017

@ViktorHofer ViktorHofer added area-System.Runtime.Extensions bug Product bug (most likely) labels May 12, 2017
@ViktorHofer ViktorHofer added this to the 2.0.0 milestone May 12, 2017
@ViktorHofer ViktorHofer self-assigned this May 12, 2017
@ViktorHofer ViktorHofer requested a review from danmoseley May 12, 2017 04:14
@ViktorHofer ViktorHofer changed the title Fix platform ID for Mac Fix Environment.OSVersion.Platform output on MacOSX May 12, 2017
@ViktorHofer ViktorHofer force-pushed the EnvironmentPlatformID branch 2 times, most recently from 259be7a to 71f0a52 Compare May 12, 2017 04:22
@danmoseley
Copy link
Member

danmoseley commented May 12, 2017

test?

@justinvp
Copy link
Contributor

justinvp commented May 12, 2017 •
edited
Loading

I thought returning PlatformId.Unix on macOS was by-design to match Mono's behavior.

#11878
https://github.com/terrajobst/platform-compat/issues/29

@stephentoub
Copy link
Member

stephentoub commented May 12, 2017

It is by design to match Mono.

@danmoseley danmoseley reopened this May 12, 2017
@danmoseley
Copy link
Member

danmoseley commented May 12, 2017

That's fine, but @ViktorHofer could you please then add a comment to the code instead? Otherwise tihs will come up agian.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented May 12, 2017

@danmosemsft yes but I will wait till we decided if we should also add annotations. See https://github.com/dotnet/corefx/issues/19694.

@ViktorHofer ViktorHofer added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label May 12, 2017
jkotas
jkotas previously requested changes May 12, 2017
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

We should be compatible with Mono on non-Windows where possible. Since Mono is returning Unix on MacOSX, we should return it too.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented May 12, 2017

@jkotas we aren't changing it. This PR is halted till the decision is made here: https://github.com/dotnet/corefx/issues/19694

@ViktorHofer ViktorHofer force-pushed the EnvironmentPlatformID branch 2 times, most recently from 7c11842 to 7441fee Compare May 13, 2017 03:32
@ViktorHofer ViktorHofer removed * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) bug Product bug (most likely) labels May 13, 2017
@ViktorHofer ViktorHofer force-pushed the EnvironmentPlatformID branch from 7441fee to a4fd9f1 Compare May 13, 2017 03:33
@ViktorHofer
Copy link
Member Author

ViktorHofer commented May 13, 2017 •
edited
Loading

@jkotas @stephentoub @danmosemsft PTAL

jkotas
jkotas reviewed May 13, 2017
View reviewed changes
src/System.Runtime.Extensions/src/System/PlatformID.cs
{
Win32S = 0,
Win32Windows = 1,
[EditorBrowsable(EditorBrowsableState.Never)] Win32S = 0,
Copy link
Member

@jkotas jkotas May 13, 2017

Choose a reason for hiding this comment

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

It is most important to have these annotations in the reference assembly src/System.Runtime.Extensions/ref/.... The editor looks for the attributes in the reference assembly.

Annotating implementation is fine too.

Copy link
Member Author

@ViktorHofer ViktorHofer May 13, 2017

Choose a reason for hiding this comment

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

done

@ViktorHofer ViktorHofer force-pushed the EnvironmentPlatformID branch from a4fd9f1 to a6cfa45 Compare May 13, 2017 09:23
@stephentoub stephentoub merged commit 475c578 into dotnet:master May 13, 2017
@stephentoub stephentoub mentioned this pull request Jul 10, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@jkotas jkotas jkotas left review comments

@stephentoub stephentoub stephentoub approved these changes

@danmoseley danmoseley danmoseley approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ViktorHofer ViktorHofer

Labels

area-System.Runtime.Extensions

Projects

None yet

Milestone

2.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@ViktorHofer @danmoseley @justinvp @stephentoub @jkotas @dnfclas