Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Simplify property/item type requirements Fixes #6985 - #6995

#6995
Merged
rokonec merged 1 commit into
dotnet:maindotnet/msbuild:mainfrom
Forgind:matchonmetadata-outside-targetsForgind/msbuild:matchonmetadata-outside-targetsCopy head branch name to clipboard
Nov 1, 2021
Merged

Simplify property/item type requirements Fixes #6985#6995
rokonec merged 1 commit into
dotnet:maindotnet/msbuild:mainfrom
Forgind:matchonmetadata-outside-targetsForgind/msbuild:matchonmetadata-outside-targetsCopy head branch name to clipboard

Conversation

@Forgind

@Forgind Forgind commented Oct 27, 2021

Copy link
Copy Markdown
Contributor

Fixes #6985

Context

RemoveOperation currently requires either not MatchOnMetadata or that the fragments are ItemSpec<ProjectProperty, ProjectItem>.ItemExpressionFragments. The latter is how we do it in tests, but in the wild, we use ProjectPropertyInstance and ProjectItemInstance. It sounds like those two should extend the other two, but they don't. I think it would be valid to make them do so, but this is a minimal change that resolves the issue such that it passes both tests and the repro case in #6985 by essentially letting how Remove is invoked dictate what the type has to be.

Changes Made

Switch to generic P and I.

Testing

Unit tests + verified the repro case no longer repros.

@lewing

lewing commented Oct 28, 2021

Copy link
Copy Markdown
Member

This is causing us some issues with optional workloads and the sdk where we need to remove specific items outside of a target.

@Forgind

Forgind commented Oct 28, 2021

Copy link
Copy Markdown
Contributor Author

Interested in providing extra validation for the proposed fix? I'm happy to help with getting you private bits if you'd like.

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Oct 29, 2021
@rokonec
rokonec merged commit 115b58c into dotnet:main Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MatchOnMetadata not recognized on evaluation item removes (outside of target elements)

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.