[Sprint 3] Add PRD-required build properties to Directory.Build.props#63
Conversation
Closes #32 Add missing shared build properties per PRD requirements: - LangVersion=latest — always use the latest C# language version - EnableNETAnalyzers=true — enable .NET platform analyzers - AnalysisMode=All — enable all analyzer rules - EnforceCodeStyleInBuild=true — enforce .editorconfig code style at build time - CodeAnalysisTreatWarningsAsErrors=false — allow analyzer diagnostics as warnings without failing CI (TreatWarningsAsErrors remains true for compiler errors) ManagePackageVersionsCentrally=true was already added by PR #37 and lives in Directory.Build.props; not duplicated here. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds the remaining PRD-required shared .NET build/analyzer properties to the repo-wide Directory.Build.props to standardize compiler/analyzer behavior across projects.
Changes:
- Set
LangVersiontolatest - Enable .NET analyzers + full analysis mode (
EnableNETAnalyzers=true,AnalysisMode=All) - Enforce code style at build time while keeping analyzer diagnostics as warnings (
EnforceCodeStyleInBuild=true,CodeAnalysisTreatWarningsAsErrors=false)
| <PropertyGroup> | ||
| <Nullable>enable</Nullable> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <LangVersion>latest</LangVersion> |
There was a problem hiding this comment.
LangVersion=latest can make builds non-reproducible across machines/CI agents because the effective language version changes when the .NET SDK updates. To keep the intent ('newest features') while reducing churn, consider pinning to latestMajor (or an explicit version) and/or ensuring the repo pins the SDK version (e.g., via global.json) so CI and dev boxes compile with the same compiler.
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <LangVersion>latest</LangVersion> | ||
| <TreatWarningsAsErrors>true</TreatWarningsAsErrors> | ||
| <!-- Keep analyzer diagnostics as warnings so TreatWarningsAsErrors doesn't fail CI --> |
There was a problem hiding this comment.
This comment is slightly misleading: with TreatWarningsAsErrors=true, compiler warnings still fail the build, while CodeAnalysisTreatWarningsAsErrors=false is specifically about analyzer diagnostics. Consider rewording to make that distinction explicit (e.g., 'Keep analyzer diagnostics as warnings so analyzer warnings don't become errors under TreatWarningsAsErrors').
🏛️ Aragorn — Architecture ReviewVerdict: 🟡 CONDITIONAL APPROVE (needs build confirmation, then merge) Gate Checklist
Code QualityThe diff is minimal and correct (6 lines added to
The comment Risk:
|
|
Aragorn — PR Gate Review: CONDITIONAL APPROVE ✅ This PR is ready to merge pending ONE local verification step: dotnet build MyBlog.slnx --configuration ReleasePlease confirm the build completes with 0 errors (warnings OK). Once verified locally, this can be merged. Systemic note: |
…uild-props-prd-properties
github-actions
Bot
added
the
squad
🏗️ PR Added to Squad Triage QueueThis PR has been labeled with Next steps:
|
SummarySummary
CoverageAppHost - 0%
Domain - 87.2%
ServiceDefaults - 0%
Web - 68.7%
|
Summary
Closes #32
Working as Sam (Backend Developer)
Adds the remaining PRD-required shared build properties to
Directory.Build.props.Changes
LangVersionlatestEnableNETAnalyzerstrueAnalysisModeAllEnforceCodeStyleInBuildtrue.editorconfigstyle at build timeCodeAnalysisTreatWarningsAsErrorsfalseTreatWarningsAsErrors=trueManagePackageVersionsCentrally=true(PR #37) is not duplicated.Acceptance Criteria
Directory.Build.propscontains all PRD-required shared build propertiesCodeAnalysisTreatWarningsAsErrors=falseprevents analyzer rules from breaking CI.csprojoverrides conflict with the new propertiesdotnet build MyBlog.slnx --configuration Releasepasses (CI validates)