feat: Reorganized solution and cleanedup the configurations#183
Conversation
samtrion
added
the
state:ready for merge
samtrion
added
the
type:feature
WalkthroughThe PR centralizes MSBuild props/targets into a new buildMultiTargeting layer, updates transitive imports to reference it, removes certain VS-only generated-file rules, reformats one ProjectReference, adds EditorConfig max_line_length = 160 for XML project files, and substantially expands README and diagnostic/documentation markdowns. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Knowledge base: Disabled due to 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (4)
🧰 Additional context used🪛 LanguageToolREADME.md[uncategorized] ~114-~114: The official name of this software platform is spelled with a capital “H”. (GITHUB) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (12)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NetEvolve.Defaults/build/NetEvolve.Defaults.targets (1)
2-2: Thin forwarding to multi-targeting targets is reasonableTurning this into a simple proxy to
..\buildMultiTargeting\NetEvolve.Defaults.targetskeeps the NuGetbuildsurface stable while centralizing logic in one place. The relative path pattern is fine; if you touch this again, you could also use$(MSBuildThisFileDirectory)..\buildMultiTargeting\...for slightly cleaner concatenation, but it’s not required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (10)
.editorconfig(1 hunks)src/NetEvolve.Defaults/NetEvolve.Defaults.csproj(1 hunks)src/NetEvolve.Defaults/build/NetEvolve.Defaults.props(1 hunks)src/NetEvolve.Defaults/build/NetEvolve.Defaults.targets(1 hunks)src/NetEvolve.Defaults/buildMultiTargeting/NetEvolve.Defaults.props(1 hunks)src/NetEvolve.Defaults/buildMultiTargeting/NetEvolve.Defaults.targets(1 hunks)src/NetEvolve.Defaults/buildMultiTargeting/SupportGeneral.props(0 hunks)src/NetEvolve.Defaults/buildTransitive/NetEvolve.Defaults.props(1 hunks)src/NetEvolve.Defaults/buildTransitive/NetEvolve.Defaults.targets(1 hunks)src/NetEvolve.Defaults/configurations/editorconfig.txt(1 hunks)
💤 Files with no reviewable changes (1)
- src/NetEvolve.Defaults/buildMultiTargeting/SupportGeneral.props
🔇 Additional comments (9)
src/NetEvolve.Defaults/NetEvolve.Defaults.csproj (2)
20-20: Formatting change is clean and functional.The ProjectReference has been properly reformatted to a single line without any impact on behavior. The
PrivateAssets="None"setting is appropriate for ensuring the analyzer is available to package consumers.
14-14: Correct inclusion of new buildMultiTargeting directory in package.The new
buildMultiTargeting\**entry is properly included in theNoneItemGroup for packaging, aligning with the PR's restructuring objective to centralize build configuration under this new layer.src/NetEvolve.Defaults/configurations/editorconfig.txt (1)
47-47: Verify this change complies with the documented process.Line 5 explicitly states: "DO NOT CHANGE SETTINGS IN THIS FILE. PLEASE CREATE PULL REQUEST IN REPOSITORY
dotnet-engineering." However, this PR modifies line 47. Clarify whether:
- The process documented on line 5 has changed and the header should be updated, or
- This change should have been submitted to the
dotnet-engineeringrepository instead.Additionally, this identical change is duplicated in
.editorconfig; consider consolidating the source of truth or documenting why both files must be maintained separately..editorconfig (1)
47-47: Process concern: File header contradicts change being made.Like
src/NetEvolve.Defaults/configurations/editorconfig.txt, line 5 states: "DO NOT CHANGE SETTINGS IN THIS FILE. PLEASE CREATE PULL REQUEST IN REPOSITORYdotnet-engineering." Verify that this change has followed the appropriate process or update the header if the policy has changed.Additionally, this change is duplicated across two files. Clarify the intended relationship: should the nested
configurations/editorconfig.txtinherit/override settings from this root file, or are they maintained independently?src/NetEvolve.Defaults/buildMultiTargeting/NetEvolve.Defaults.targets (1)
2-22: Multi-targeting split looks sound; confirm flag naming consistencyBreaking the monolithic import into explicit support imports with per-feature conditions reads clean and gives good control. Paths using
$(MSBuildThisFileDirectory)Support*.targetsare correct given MSBuild’s trailing-directory behavior.Only soft ask: double-check that the new/relocated flags (
DisableSupportPackageInformation,DisableSupportCopyrightTargets,DisableSupportNuGetAuditTargets,IsTestableProject) match the names you actually set in consuming projects and any docs, so you don’t silently change behavior when this ships.src/NetEvolve.Defaults/buildTransitive/NetEvolve.Defaults.props (1)
2-2: Transitive props now aligned with centralized multi-targeting propsPointing
buildTransitiveat..\buildMultiTargeting\NetEvolve.Defaults.propsis consistent with the rest of the reorg and should ensure transitive consumers pick up the same defaults as direct ones. The relative path and directory step-up look correct.src/NetEvolve.Defaults/build/NetEvolve.Defaults.props (1)
2-2: Centralizingbuildprops through multi-targeting hub looks correctHaving
build/NetEvolve.Defaults.propsimport..\buildMultiTargeting\NetEvolve.Defaults.propskeeps the package’s primary props surface while moving all logic into the centralized multi-targeting file. This should make future tweaks much easier without changing the NuGet-facing entry point.src/NetEvolve.Defaults/buildMultiTargeting/NetEvolve.Defaults.props (1)
2-7: Central props hub is coherent and matches targets’ behaviorImporting
SupportGeneral.propsand CI detection unconditionally, withSupportPackageInformation.propsbehindDisableSupportPackageInformation, is a clear and flexible setup. This also mirrors the condition used forSupportPackageInformation.targets, so props and targets for that feature will be enabled/disabled together, which avoids subtle mismatches.src/NetEvolve.Defaults/buildTransitive/NetEvolve.Defaults.targets (1)
2-2: Transitive targets correctly point at the new multi-targeting entryRouting
buildTransitive/NetEvolve.Defaults.targetsto..\buildMultiTargeting\NetEvolve.Defaults.targetsmatches the pattern used forbuild/and keeps transitive behavior aligned with direct references after the reorg. The relative path looks correct.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #183 +/- ##
===========================
===========================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.