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.

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

Rename SdkSupportedTargetPlatform, add SupportedTargetPlatform #12872

Merged
merged 5 commits into from
Aug 24, 2020

Conversation

sfoslund
Copy link
Member

@sfoslund sfoslund commented Aug 11, 2020

Fixes #12706

cc @terrajobst

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.

We should name the group SupportedPlatform but other than that, it looks good.

@terrajobst
Copy link
Contributor

I've talked @pranavkm, he'll make sure the Blazor SDK will do this in their .props file:

<ItemGroup>
    <SupportedPlatform Remove="@(SupportedPlatform)" />
    <SupportedPlatform Include="browser" />
</ItemGroup>

For more details, see the spec.

@terrajobst
Copy link
Contributor

terrajobst commented Aug 17, 2020

@sfoslund @dsplaisted will this be merged before 3?

@terrajobst
Copy link
Contributor

terrajobst commented Aug 17, 2020

@sfoslund @dsplaisted will this be merged before 3?

Let's not do a fire drill this week. But we should get it in soon-ish.

Experimenting to see what happens if I edit a different repo
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

@dsplaisted would you mind reviewing this?

pranavkm added a commit to dotnet/aspnetcore that referenced this pull request Aug 19, 2020
@wli3
Copy link

wli3 commented Aug 19, 2020

Need to adjust against #12775

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm not sure what @wli3 means by this:

Need to adjust against #12775

So I'd make sure to clarify that first

@sfoslund
Copy link
Member Author

will this be merged before 3?

@terrajobst sorry, I was OOF at the beginning of this week. I'll update based on feedback now and get this in this week.

@jeffhandley
Copy link
Member

I think @pranavkm mentioned he was going to take a look at this too--but I'm not sure if he took any actions on it yet.

@pranavkm
Copy link
Contributor

I edited some of these files, but looks like @sfoslund's got a handle on this 👍

@sfoslund
Copy link
Member Author

@dsplaisted @terrajobst does this look okay to you?

@dsplaisted
Copy link
Member

@dsplaisted @terrajobst does this look okay to you?

Yes, but I'd like clarification from @wli3 on what he meant in this comment

@wli3
Copy link

wli3 commented Aug 20, 2020

Now I look it in detail. This change has nothing to do with the attribute "SupportedOSPlatformAttribute" in #12775 right? And this rename is to avoid that name conflict. If so, this is good.

@terrajobst
Copy link
Contributor

Now I look it in detail. This change has nothing to do with the attribute "SupportedOSPlatformAttribute" in #12775 right? And this rename is to avoid that name conflict. If so, this is good

Logically related, but independent. This item group is only used by our analyzer.

@mavasani what is required to pass this down? Do we need to register the item group somewhere? Or did you do that already?

@mavasani
Copy link
Contributor

I presume you mean: dotnet/roslyn-analyzers#3950 (comment)?

@sfoslund
Copy link
Member Author

@mavasani things are good on your side for this to go in then?

@mavasani
Copy link
Contributor

@mavasani things are good on your side for this to go in then?

I am not sure I understand the changes in this PR, but updating the analyzers to understand any name chosen here would be a trivial change, so in that sense this PR does not break or hinder the analyzers in anyway.

@sfoslund
Copy link
Member Author

Great, sounds like this is good to go in then. I'll merge it today unless there are any objections.

@sfoslund sfoslund merged commit 5615589 into dotnet:master Aug 24, 2020
@sfoslund sfoslund deleted the SupportedTargetPlatform branch August 24, 2020 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a standard item group for OS platforms
7 participants