Skip to content

fix(cleanup): enhance FullCleanUp target for cross-targeting projects#386

Merged
samtrion merged 1 commit intomainfrom
fix/multi-targetframeworks-support-full-cleanup
Apr 20, 2026
Merged

fix(cleanup): enhance FullCleanUp target for cross-targeting projects#386
samtrion merged 1 commit intomainfrom
fix/multi-targetframeworks-support-full-cleanup

Conversation

@samtrion
Copy link
Copy Markdown
Contributor

@samtrion samtrion commented Apr 20, 2026

Summary by CodeRabbit

  • Bug Fixes
    • Fixed build cleanup behavior for multi-targeted projects to prevent unintended cleanup during cross-targeting builds.

@samtrion samtrion self-assigned this Apr 20, 2026
@samtrion samtrion requested a review from a team as a code owner April 20, 2026 20:32
@samtrion samtrion added state:ready for merge Indicates that a pull request has been reviewed and approved, and is ready to be merged into the mai type:bug Indicates an issue or flaw that needs to be fixed. labels Apr 20, 2026
@samtrion samtrion requested review from Hnogared and removed request for a team April 20, 2026 20:32
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 20, 2026

Walkthrough

An MSBuild targets file was updated to introduce conditional execution logic for the FullCleanUp target. A new IsCrossTargetingProject property detects multi-targeting scenarios, and the cleanup target now executes only when this property matches IsCrossTargetingBuild.

Changes

Cohort / File(s) Summary
MSBuild Configuration
src/NetEvolve.Defaults/buildMultiTargeting/SupportFullCleanUp.targets
Added TreatAsLocalProperty="IsCrossTargetingProject" attribute to Project declaration. Introduced IsCrossTargetingProject property that evaluates to true when $(TargetFrameworks) is non-empty. Gated FullCleanUp target execution with a condition requiring IsCrossTargetingProject to equal IsCrossTargetingBuild.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A rabbit's rhyme on targets clean,
Cross-targeting builds, a conditional dream,
Properties dance and conditions align,
MSBuild logic, oh so fine!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: enhancing the FullCleanUp target with a condition to handle cross-targeting projects correctly.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/NetEvolve.Defaults/buildMultiTargeting/SupportFullCleanUp.targets (1)

3-3: Quote the TargetFrameworks expansion in the condition.

TargetFrameworks can expand to semicolon-delimited TFMs, so keep the condition parser-safe and consistent with Line 5 by quoting the property value. Microsoft’s MSBuild condition docs also recommend quoting strings/property values in conditions except for simple alphanumeric/boolean values: https://learn.microsoft.com/visualstudio/msbuild/msbuild-conditions

♻️ Proposed fix
-    <IsCrossTargetingProject Condition="$(TargetFrameworks) != ''">true</IsCrossTargetingProject>
+    <IsCrossTargetingProject Condition="'$(TargetFrameworks)' != ''">true</IsCrossTargetingProject>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/NetEvolve.Defaults/buildMultiTargeting/SupportFullCleanUp.targets` at
line 3, The condition for setting IsCrossTargetingProject uses an unquoted
property expansion "$(TargetFrameworks)" which can contain semicolons and should
be quoted for MSBuild condition-safety; update the Condition on the
IsCrossTargetingProject element to quote the TargetFrameworks expansion (e.g.,
change Condition="$(TargetFrameworks) != ''" to use a quoted property) so it
matches the quoting style used elsewhere (see the other occurrence on Line 5)
and avoids parsing issues with multi-TFM values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/NetEvolve.Defaults/buildMultiTargeting/SupportFullCleanUp.targets`:
- Line 3: The condition for setting IsCrossTargetingProject uses an unquoted
property expansion "$(TargetFrameworks)" which can contain semicolons and should
be quoted for MSBuild condition-safety; update the Condition on the
IsCrossTargetingProject element to quote the TargetFrameworks expansion (e.g.,
change Condition="$(TargetFrameworks) != ''" to use a quoted property) so it
matches the quoting style used elsewhere (see the other occurrence on Line 5)
and avoids parsing issues with multi-TFM values.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2f26b02d-f3ae-4fe4-8221-fae547f1c693

📥 Commits

Reviewing files that changed from the base of the PR and between ce6f1f7 and 91af6b1.

📒 Files selected for processing (1)
  • src/NetEvolve.Defaults/buildMultiTargeting/SupportFullCleanUp.targets

@samtrion samtrion merged commit e988f9c into main Apr 20, 2026
12 checks passed
@samtrion samtrion deleted the fix/multi-targetframeworks-support-full-cleanup branch April 20, 2026 20:49
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (b11bae5) to head (91af6b1).
⚠️ Report is 202 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##              main   #386        +/-   ##
===========================================
- Coverage   100.00%      0   -100.00%     
===========================================
  Files            1      0         -1     
  Lines            1      0         -1     
===========================================
- Hits             1      0         -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@samtrion samtrion restored the fix/multi-targetframeworks-support-full-cleanup branch April 20, 2026 21:09
@samtrion samtrion deleted the fix/multi-targetframeworks-support-full-cleanup branch April 20, 2026 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state:ready for merge Indicates that a pull request has been reviewed and approved, and is ready to be merged into the mai type:bug Indicates an issue or flaw that needs to be fixed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant