Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add platform-specific attributes #38604

Merged
merged 9 commits into from
Jul 1, 2020
Merged

Add platform-specific attributes #38604

merged 9 commits into from
Jul 1, 2020

Conversation

wli3
Copy link

@wli3 wli3 commented Jun 30, 2020

Spec #33331

@Dotnet-GitSync-Bot
Copy link
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
Copy link
Author

wli3 commented Jun 30, 2020

@terrajobst @mhutch @jeffhandley

@wli3
Copy link
Author

wli3 commented Jun 30, 2020

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
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

LGTM otherwise.

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MinimumOSAttribute.cs Outdated Show resolved Hide resolved
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 5.0.0 milestone Jun 30, 2020
@jeffhandley
Copy link
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
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MinimumOSAttribute.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MinimumOSAttribute.cs Outdated Show resolved Hide resolved
...es/System.Private.CoreLib/src/System/Runtime/InteropServices/ObsoletedInPlatformAttribute.cs Outdated Show resolved Hide resolved
...es/System.Private.CoreLib/src/System/Runtime/InteropServices/ObsoletedInPlatformAttribute.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PlatformAttribute.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PlatformAttribute.cs Outdated Show resolved Hide resolved
...ries/System.Private.CoreLib/src/System/Runtime/InteropServices/RemovedInPlatformAttribute.cs Outdated Show resolved Hide resolved
...ries/System.Private.CoreLib/src/System/Runtime/InteropServices/RemovedInPlatformAttribute.cs Outdated Show resolved Hide resolved
@jeffhandley jeffhandley requested a review from buyaa-n June 30, 2020 06:22
stephentoub
stephentoub reviewed Jun 30, 2020
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MinimumOSAttribute.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PlatformAttribute.cs Outdated Show resolved Hide resolved
...es/System.Private.CoreLib/src/System/Runtime/InteropServices/ObsoletedInPlatformAttribute.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PlatformAttribute.cs Outdated Show resolved Hide resolved
...ries/System.Private.CoreLib/src/System/Runtime/InteropServices/RemovedInPlatformAttribute.cs Outdated Show resolved Hide resolved
...braries/System.Private.CoreLib/src/System/Runtime/InteropServices/TargetPlatformAttribute.cs Outdated Show resolved Hide resolved
terrajobst
terrajobst suggested changes Jun 30, 2020
View reviewed changes
Copy link
Contributor

@terrajobst terrajobst left a comment

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
Copy link
Author

wli3 commented Jun 30, 2020

I resolved all but the unit tests.

@wli3
Copy link
Author

wli3 commented Jun 30, 2020

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
Copy link
Contributor

buyaa-n commented Jul 1, 2020

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
Copy link
Author

wli3 commented Jul 1, 2020

@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
Copy link
Author

wli3 commented Jul 1, 2020

🎉 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

@terrajobst terrajobst terrajobst approved these changes

@bartonjs bartonjs bartonjs left review comments

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT left review comments

@buyaa-n buyaa-n buyaa-n left review comments

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