feat(cleanup): add full cleanup target to remove intermediate and out…#385
feat(cleanup): add full cleanup target to remove intermediate and out…#385
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 41 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA new MSBuild target file Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/NetEvolve.Defaults/buildMultiTargeting/SupportFullCleanUp.targets (1)
2-3: Reconsider automatic full cleanup on standardCleaninvocation.
AfterTargets="Clean"automatically triggers full cleanup when any user invokesdotnet clean. While this repository's own workflows don't useclean, this is a distributed package consumed by many downstream projects. Some consumers may have workflows or development practices that combinecleanwith subsequent builds, and the automatic removal ofBaseIntermediateOutputPath(includingproject.assets.json) could breakbuild/test --no-restorepatterns in those projects. Additionally, consumers who customizeBaseOutputPathcould lose shared artifact roots unexpectedly.Consider making
FullCleanUpan explicit opt-in target (requiring explicit/t:FullCleanUpinvocation) rather than a side effect of standardclean:Proposed fix
- <Target Name="FullCleanUp" AfterTargets="Clean"> + <Target Name="FullCleanUp"> <RemoveDir Directories="$(BaseIntermediateOutputPath);$(BaseOutputPath)" /> </Target>Alternatively, gate the behavior behind an opt-in property like
<EnableFullCleanUp>false</EnableFullCleanUp>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NetEvolve.Defaults/buildMultiTargeting/SupportFullCleanUp.targets` around lines 2 - 3, The FullCleanUp target is currently bound to run automatically via AfterTargets="Clean" and removes $(BaseIntermediateOutputPath) and $(BaseOutputPath) via RemoveDir; change this to an opt-in action by either removing the AfterTargets="Clean" attribute so FullCleanUp only runs when explicitly invoked (/t:FullCleanUp) or add a gate property such as EnableFullCleanUp and wrap the AfterTargets or the RemoveDir call in a Condition that checks "'$(EnableFullCleanUp)' == 'true'"; update the target definition (Target Name="FullCleanUp") and the RemoveDir invocation accordingly so downstream consumers aren’t surprised by automatic deletion of BaseIntermediateOutputPath or custom BaseOutputPath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NetEvolve.Defaults/buildMultiTargeting/NetEvolve.Defaults.targets`:
- Line 12: The import of SupportFullCleanUp.targets is unconditional and
forcibly alters Clean for all consumers; add a property-based opt-out so
consumers can disable it. Modify the Import of SupportFullCleanUp.targets to
include a Condition that allows opt-out (for example,
Condition="'$(EnableNetEvolveSupportFullCleanUp)' != 'false'" or an inverse
opt-out like Condition="'$(DisableNetEvolveSupportFullCleanUp)' != 'true'"), and
document that consumers can set that MSBuild property to opt out of the
destructive Clean behavior introduced by SupportFullCleanUp.targets (referencing
the Import line and the Clean target affected).
---
Nitpick comments:
In `@src/NetEvolve.Defaults/buildMultiTargeting/SupportFullCleanUp.targets`:
- Around line 2-3: The FullCleanUp target is currently bound to run
automatically via AfterTargets="Clean" and removes $(BaseIntermediateOutputPath)
and $(BaseOutputPath) via RemoveDir; change this to an opt-in action by either
removing the AfterTargets="Clean" attribute so FullCleanUp only runs when
explicitly invoked (/t:FullCleanUp) or add a gate property such as
EnableFullCleanUp and wrap the AfterTargets or the RemoveDir call in a Condition
that checks "'$(EnableFullCleanUp)' == 'true'"; update the target definition
(Target Name="FullCleanUp") and the RemoveDir invocation accordingly so
downstream consumers aren’t surprised by automatic deletion of
BaseIntermediateOutputPath or custom BaseOutputPath.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8cf96c5c-2f2e-4b4d-9e4d-420af4f64883
📒 Files selected for processing (2)
src/NetEvolve.Defaults/buildMultiTargeting/NetEvolve.Defaults.targetssrc/NetEvolve.Defaults/buildMultiTargeting/SupportFullCleanUp.targets
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Martin Stühmer <me@samtrion.net>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #385 +/- ##
===========================
===========================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…put directories
Summary by CodeRabbit