Cleanup project files; configure Directory.Build.props #628
Conversation
📝 WalkthroughWalkthroughCentralizes MSBuild packaging, signing, and analyzer settings in Directory.Build.props/targets; removes per-project AssemblyInfo files; updates many .csproj files to rely on centralized properties, adjust IsPackable/signing/COM settings, and change conditional compilation symbols and test/resource configurations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer / CI
participant MSBuild as MSBuild (dotnet build / pack)
participant Props as Directory.Build.props
participant Targets as Directory.Build.targets
participant Project as Individual .csproj
participant NuGet as NuGet/publish
Dev->>MSBuild: run build/pack
MSBuild->>Props: import global properties
MSBuild->>Project: apply project file (overrides)
MSBuild->>Targets: import global targets
Targets->>Project: inject defines (SYSTEM_DRAWING,HAS_SPAN) and signing rules
Project->>MSBuild: evaluated build graph
alt IsPackable == true
MSBuild->>NuGet: pack using Package metadata (README/icon included)
NuGet-->>MSBuild: package created
else
MSBuild-->>Dev: build output only
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ 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). (2)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
QRCoderConsole/Program.cs (1)
164-174: Define theWINDOWSsymbol in QRCoderConsole.csproj
There’s noWINDOWSconstant defined, so the#if WINDOWSblock (Program.cs lines 164–174) will never be included. Under the net5.0-windows and net6.0-windows<PropertyGroup>in QRCoderConsole.csproj add:<DefineConstants>WINDOWS;$(DefineConstants)</DefineConstants>or revert to using the built-in
NET5_0_WINDOWS | NET6_0_WINDOWSsymbols.
♻️ Duplicate comments (1)
QRCoderConsole/Program.cs (1)
238-240: Same symbol definition concern as lines 12-14.The conditional compilation change here mirrors the one at lines 12-14. Ensure the
NET6_0andWINDOWSsymbols are properly defined (see comment on lines 12-14).
🧹 Nitpick comments (1)
QRCoder/QRCoder.csproj (1)
8-8: Redundant NoWarn directive.The
CS1591warning is already suppressed inDirectory.Build.propsline 40. This duplicate suppression can be removed.- <NoWarn>$(NoWarn);CS1591</NoWarn>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
QRCoder.Xaml/Assets/nuget-icon-xaml.pngis excluded by!**/*.pngnuget-icon.pngis excluded by!**/*.png
📒 Files selected for processing (17)
Directory.Build.props(1 hunks)Directory.Build.targets(1 hunks)QRCoder.Xaml/Properties/AssemblyInfo.cs(0 hunks)QRCoder.Xaml/QRCoder.Xaml.csproj(2 hunks)QRCoder.sln(1 hunks)QRCoder/Properties/AssemblyInfo.cs(0 hunks)QRCoder/QRCoder.csproj(1 hunks)QRCoderApiTests/QRCoderApiTests.csproj(0 hunks)QRCoderConsole/DataObjects/SupportedImageFormat.cs(1 hunks)QRCoderConsole/Program.cs(3 hunks)QRCoderConsole/Properties/AssemblyInfo.cs(0 hunks)QRCoderConsole/QRCoderConsole.csproj(0 hunks)QRCoderDemo/Properties/AssemblyInfo.cs(0 hunks)QRCoderDemo/QRCoderDemo.csproj(1 hunks)QRCoderDemoUWP/Properties/AssemblyInfo.cs(0 hunks)QRCoderTests/QRCoderTests.csproj(2 hunks)QRCoderTrimAnalysis/QRCoderTrimAnalysis.csproj(0 hunks)
💤 Files with no reviewable changes (8)
- QRCoderTrimAnalysis/QRCoderTrimAnalysis.csproj
- QRCoderDemoUWP/Properties/AssemblyInfo.cs
- QRCoderConsole/QRCoderConsole.csproj
- QRCoder.Xaml/Properties/AssemblyInfo.cs
- QRCoderApiTests/QRCoderApiTests.csproj
- QRCoderConsole/Properties/AssemblyInfo.cs
- QRCoderDemo/Properties/AssemblyInfo.cs
- QRCoder/Properties/AssemblyInfo.cs
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: additional-tests
🔇 Additional comments (9)
QRCoder.sln (1)
28-28: LGTM!Adding
Directory.Build.targetsto the Solution Items maintains consistency with the existing pattern forDirectory.Build.propsand improves visibility of build configuration files in Visual Studio.QRCoderDemo/QRCoderDemo.csproj (1)
1-25: LGTM!The cleanup changes align with the centralized build configuration strategy. Removing redundant settings and empty property groups improves maintainability without affecting functionality.
Directory.Build.targets (1)
1-15: LGTM with note on WINDOWS constant.The conditional compilation constants and signing configuration look correct:
SYSTEM_DRAWINGappropriately excludes frameworks without built-in System.Drawing supportHAS_SPANcorrectly targets frameworks with Span support- Strong-name signing is properly scoped to Release builds of packable projects
However, note that this file does not define a
WINDOWSconstant, which is referenced inQRCoderConsole/DataObjects/SupportedImageFormat.csline 11. That may need to be addressed (see comment on that file).QRCoderTests/QRCoderTests.csproj (1)
1-61: LGTM!The changes improve maintainability:
- Conditional
RuntimeFrameworkVersionis properly scoped to netcoreapp2.1- Wildcard patterns for embedded resources (
assets/*.svgandassets/*.png) are cleaner than explicit file listings- Removal of redundant properties aligns with centralized configuration
QRCoder.Xaml/QRCoder.Xaml.csproj (2)
5-5: LGTM!The condition correctly applies WPF to both net5.0-windows and net6.0-windows targets.
28-30: LGTM!The package-specific readme is properly configured with the
None Updateitem.Directory.Build.props (3)
3-11: LGTM!The comment block clearly documents the MSBuild property evaluation order, which is helpful for understanding how Directory.Build.props/targets interact with individual project files.
12-43: LGTM! Consider clarifying analyzer configuration.The centralized build and packaging configuration is well-structured and comprehensive. The combination of
EnableNETAnalyzers=TruewithAnalysisMode=None(lines 41-42) is unusual but may be intentional to disable default rulesets while allowing explicit analyzer configuration in individual projects or.editorconfig.If the intent is to use custom analyzer rules, consider adding a comment explaining why
AnalysisMode=Noneis used withEnableNETAnalyzers=True.
46-49: LGTM!Conditional inclusion of README and icon for packable projects is well-structured and uses proper MSBuild properties.
| [assembly: AssemblyTitle("QRCoder")] | ||
| [assembly: AssemblyDescription("QRCoder is a simple library, written in C#.NET, which enables you to create QR Codes. It's licensed under the MIT-license.")] | ||
| [assembly: AssemblyConfiguration("")] | ||
| [assembly: AssemblyCompany("www.code-bude.net")] | ||
| [assembly: AssemblyProduct("QRCoder")] | ||
| [assembly: AssemblyCopyright("Copyright © www.code-bude.net / Raffael Herrmann. All rights reserved.")] | ||
| [assembly: AssemblyTrademark("written by Raffael Herrmann")] | ||
| [assembly: AssemblyCulture("")] |
There was a problem hiding this comment.
I'm concentrating on what appears in NuGet, not what's compiled into the assembly. So some of these have been omitted in Directory.Build.props because NuGet ignores them, or because they already default to the correct value.
| [assembly: ComVisible(true)] | ||
|
|
||
| // Die folgende GUID bestimmt die ID der Typbibliothek, wenn dieses Projekt für COM verfügbar gemacht wird | ||
| [assembly: Guid("e668b98b-83bb-4e60-b33c-4fd5ed9c0156")] |
There was a problem hiding this comment.
The old GUID for COM was incorrectly set the same as the QRCoder's GUID. A new GUID has been assigned for QRCoder.Xaml.
Summary by CodeRabbit
Packaging
Signing
Platform Support
Quality & CI
Cleanup