Skip to content

Use TargetFramework conditions consistently in libraries - #35956

Merged
Anipik merged 5 commits into
dotnet:masterfrom
ViktorHofer:TfmConditions
May 11, 2020
Merged

Use TargetFramework conditions consistently in libraries#35956
Anipik merged 5 commits into
dotnet:masterfrom
ViktorHofer:TfmConditions

Conversation

@ViktorHofer

@ViktorHoferViktorHofer commented May 7, 2020•
edited
Loading

Copy link
Copy Markdown
Member

See the docs commit for the desired behavior: b0b09c6.

Additional cleanup:

  • Remove unnecessary properties (Rootnamespace, AssemblyName)
  • Move TargetFramework derived properties (or ones that will dervice from TargetFramework at some point) into a/the second property group.

cc @ericstj

@ViktorHofer
ViktorHofer requested review from Anipik and safernMay 7, 2020 11:45
@ghost

ghost commented May 7, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

ericstj
ericstj reviewed May 7, 2020
View reviewed changes
Comment threaddocs/coding-guidelines/project-guidelines.md

# Library project guidelines

## TargetFramework conditions

@ericstjericstjMay 7, 2020

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.

Should we call out that $(TargetFramework) should be avoided in PropertyGroup conditions (unless those are in a second PropertyGroup)?

@ViktorHoferViktorHoferMay 8, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Thanks, done :)

@ericstjericstjMay 8, 2020

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.

Do you know if we're going to validate that? I recall at one point we talked about it.

@ViktorHoferViktorHoferMay 8, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I would prefer to not add infra for that and instead push on the project system / msbuild team to make progress on the actual issue.

@jaredparjaredpar mentioned this pull request May 7, 2020
Closed
@ViktorHofer

ViktorHofer commented May 8, 2020

Copy link
Copy Markdown
MemberAuthor

Feedback is addressed. Someone pls approve :D

Anipik
Anipik reviewed May 8, 2020
View reviewed changes
Comment threadsrc/libraries/System.IO.Packaging/ref/System.IO.Packaging.csproj
@ViktorHofer

ViktorHofer commented May 9, 2020

Copy link
Copy Markdown
MemberAuthor

Ping reviewers, please approve :)

@Anipik
Anipik merged commit 2cda903 into dotnet:masterMay 11, 2020
@ghostghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ericstjericstjericstj approved these changes
@safernsafernsafern approved these changes
+1 more reviewer
@AnipikAnipikAnipik approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-Infrastructure

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ViktorHofer@ericstj@Anipik@safern@Dotnet-GitSync-Bot