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

Include diagnostic level metadata on ResolvedPackageReference #12069

Merged

Conversation

drewnoakes
Copy link
Member

Contributes to dotnet/project-system#6275.

This information is needed by the Project System to correctly indicate when a package in the tree has an associated warning or error state.

This information is needed by the Project System to correctly indicate when a package in the tree has an associated warning or error state.
@drewnoakes
Copy link
Member Author

The consumption of this data is in dotnet/project-system#6288.

@drewnoakes drewnoakes force-pushed the include-library-diagnostic-level branch from ff326ab to 6f5f5c1 Compare June 16, 2020 23:51
Copy link

@wli3 wli3 left a comment

Choose a reason for hiding this comment

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

Other than the comment looks good

@@ -119,6 +119,8 @@ public string ProjectLanguage
/// </summary>
public bool EmitLegacyAssetsFileItems { get; set; } = false;

public string TargetFrameworkMoniker { get; set; }
Copy link

Choose a reason for hiding this comment

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

only Diagnostic is relevant to TargetFrameworkMoniker? Should "PackageDependencies" be different when TargetFrameworkMoniker is different? (Although "Diagnostic" is the only thing TargetFrameworkMoniker applied to.)

Copy link

Choose a reason for hiding this comment

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

If so, maybe we should call it something like DiagnosticTargetFrameworkMoniker

Copy link
Member Author

Choose a reason for hiding this comment

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

The input items have already filtered by TargetFrameworkMoniker in ResolvePackageDependencies. I like keeping this property name the same as the MSBuild property that it comes from. The fact that it's currently only used for diagnostics is kind of an implementation detail.

@wli3
Copy link

wli3 commented Jun 17, 2020

The CI looks stuck. You can git commit --amend and force push it to restart the CI

@drewnoakes
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@drewnoakes
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@drewnoakes drewnoakes merged commit 7594743 into dotnet:release/3.1.4xx Jun 17, 2020
@drewnoakes drewnoakes deleted the include-library-diagnostic-level branch June 17, 2020 22:47
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.

2 participants