Skip to content

Add platform-specific attributes#38604

Merged
jeffhandley merged 9 commits into
dotnet:masterfrom
wli3:dev/wul/add-in-platform-specific-attribute
Jul 1, 2020
Merged

Add platform-specific attributes#38604
jeffhandley merged 9 commits into
dotnet:masterfrom
wli3:dev/wul/add-in-platform-specific-attribute

Conversation

@wli3

@wli3 wli3 commented Jun 30, 2020

Copy link
Copy Markdown

Spec #33331

@Dotnet-GitSync-Bot

Dotnet-GitSync-Bot commented Jun 30, 2020

Copy link
Copy Markdown
Collaborator

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.

@wli3

wli3 commented Jun 30, 2020

Copy link
Copy Markdown
Author

@terrajobst @mhutch @jeffhandley

@wli3

wli3 commented Jun 30, 2020

Copy link
Copy Markdown
Author

Is the branch right to get in net5.0? Should I keep the comments? These files in the right location?

AaronRobinsonMSFT
AaronRobinsonMSFT reviewed Jun 30, 2020
View reviewed changes

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM otherwise.

Comment thread src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MinimumOSAttribute.cs Outdated
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 5.0.0 milestone Jun 30, 2020
@jeffhandley

jeffhandley commented Jun 30, 2020

Copy link
Copy Markdown
Member

@wli3 Thanks! I'm reviewing. I was commenting with some suggestions to convert to XML docs, and I'm also rewriting some of the comments to be more docs-ready than what was captured in the spec, and I saw you've already converted to XML comments..

I don't think we did a great job on the spec review capturing final names, so I'm going to call in @terrajobst on my review to confirm a couple details there too. We'll want to get @bartonjs's review too.

And yes, this is the right branch to make net5.0.

jeffhandley
jeffhandley reviewed Jun 30, 2020
View reviewed changes
Comment thread src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MinimumOSAttribute.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MinimumOSAttribute.cs Outdated
Comment thread ...es/System.Private.CoreLib/src/System/Runtime/InteropServices/ObsoletedInPlatformAttribute.cs
Comment thread ...es/System.Private.CoreLib/src/System/Runtime/InteropServices/ObsoletedInPlatformAttribute.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PlatformAttribute.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PlatformAttribute.cs
Comment thread ...ries/System.Private.CoreLib/src/System/Runtime/InteropServices/RemovedInPlatformAttribute.cs Outdated
Comment thread ...ries/System.Private.CoreLib/src/System/Runtime/InteropServices/RemovedInPlatformAttribute.cs Outdated
@jeffhandley jeffhandley requested a review from buyaa-n June 30, 2020 06:22
stephentoub
stephentoub reviewed Jun 30, 2020
View reviewed changes
Comment thread src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MinimumOSAttribute.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PlatformAttribute.cs Outdated
Comment thread ...es/System.Private.CoreLib/src/System/Runtime/InteropServices/ObsoletedInPlatformAttribute.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PlatformAttribute.cs Outdated
Comment thread ...ries/System.Private.CoreLib/src/System/Runtime/InteropServices/RemovedInPlatformAttribute.cs Outdated
Comment thread ...braries/System.Private.CoreLib/src/System/Runtime/InteropServices/TargetPlatformAttribute.cs
terrajobst
terrajobst suggested changes Jun 30, 2020
View reviewed changes

@terrajobst terrajobst left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I left some comments on the naming/API shape. I have updated the review comment to include the agreed upon shape for all APIs. #33331 (comment)

William Li and others added 3 commits June 30, 2020 10:54
@bartonjs
Update src/libraries/System.Private.CoreLib/src/System/Runtime/Intero…
a426eb2 
…pServices/MinimumOSAttribute.cs

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
@jeffhandley
Update src/libraries/System.Private.CoreLib/src/System/Runtime/Intero…
7df9b8d 
…pServices/ObsoletedInPlatformAttribute.cs

Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
Address code review
e930a2b
@wli3

wli3 commented Jun 30, 2020

Copy link
Copy Markdown
Author

I resolved all but the unit tests.

@wli3

wli3 commented Jun 30, 2020

Copy link
Copy Markdown
Author

I cannot push to dotnet/runtime so i send the PR from my own fork. Please feel free to start a different PR if you cannot push to my branch

@jaredpar jaredpar mentioned this pull request Jun 30, 2020
Closed
@buyaa-n buyaa-n force-pushed the dev/wul/add-in-platform-specific-attribute branch from 8f5bda5 to c16ab65 Compare July 1, 2020 01:27
@buyaa-n

buyaa-n commented Jul 1, 2020

Copy link
Copy Markdown
Contributor

I cannot push to dotnet/runtime so i send the PR from my own fork.

@wli3 thanks! We don't push to dotnet/runtime, we send PR from our fork just as you did, i guess you have the merge capability, as all comments are resolved i think we can merge it now

@wli3

wli3 commented Jul 1, 2020

Copy link
Copy Markdown
Author

@buyaa-n I don't have the merge button either. @jeffhandley could you merge it?

@jeffhandley jeffhandley merged commit 9f3e08e into dotnet:master Jul 1, 2020
@wli3

wli3 commented Jul 1, 2020

Copy link
Copy Markdown
Author

🎉 thank you everyone!

This was referenced Jul 20, 2020
Merged
Closed
@wli3 wli3 mentioned this pull request Oct 16, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jeffhandley jeffhandley jeffhandley left review comments
@akoeplinger akoeplinger akoeplinger left review comments
@stephentoub stephentoub stephentoub left review comments
@bartonjs bartonjs bartonjs left review comments
@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT left review comments
+2 more reviewers
@terrajobst terrajobst terrajobst approved these changes
@buyaa-n buyaa-n buyaa-n left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Runtime.InteropServices

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

9 participants

@wli3 @Dotnet-GitSync-Bot @jeffhandley @buyaa-n @akoeplinger @stephentoub @terrajobst @bartonjs @AaronRobinsonMSFT