From f31cb97b97909440f90f973a3e6cdfa88734faf9 Mon Sep 17 00:00:00 2001 From: leecampbell-codeagent Date: Sun, 1 Mar 2026 14:31:41 +0000 Subject: [PATCH 1/7] plan(#116): initial brief from issue --- plan/planning/brief.md | 120 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 plan/planning/brief.md diff --git a/plan/planning/brief.md b/plan/planning/brief.md new file mode 100644 index 0000000..605ea7c --- /dev/null +++ b/plan/planning/brief.md @@ -0,0 +1,120 @@ +# Issue #116 — Add Directory.Build.props with shared build settings + +## Summary + +The repository currently has no centralised MSBuild property file. +Each of the four projects (`HdrHistogram`, `HdrHistogram.UnitTests`, `HdrHistogram.Examples`, `HdrHistogram.Benchmarking`) carries only minimal PropertyGroup settings (`TargetFramework(s)`, `OutputType`). +None of the projects configure `Nullable`, `ImplicitUsings`, `EnforceCodeStyleInBuild`, `AnalysisLevel`, or `TreatWarningsAsErrors`. + +Adding a `Directory.Build.props` file at the repository root will: + +- Centralise these settings in one place. +- Enforce `.editorconfig` style rules during build. +- Enable the built-in .NET analysers at the `latest-recommended` level. +- Establish a consistent nullable-reference-types policy across all projects. +- Avoid duplication as new projects are added. + +## Current state (confirmed by exploration) + +### Projects + +| Project | TargetFramework(s) | OutputType | Nullable | AnalysisLevel | EnforceCodeStyleInBuild | TreatWarningsAsErrors | +|---|---|---|---|---|---|---| +| HdrHistogram | net8.0;netstandard2.0 | (library) | — | — | — | NU5125;NU5048 only | +| HdrHistogram.UnitTests | net8.0 | — | — | — | — | — | +| HdrHistogram.Examples | net8.0 | Exe | — | — | — | — | +| HdrHistogram.Benchmarking | net8.0 | Exe | — | — | — | — | + +### Root configuration + +- `.editorconfig` — present, comprehensive; all code-style rules are set to `suggestion` severity. +- `Directory.Build.props` — **does not exist** (to be created by this issue). +- `Directory.Build.targets` — does not exist. +- `global.json` — does not exist. +- `nuget.config` — does not exist. +- `appveyor.yml` — does not exist. + +### Existing per-project WarningsAsErrors + +`HdrHistogram.csproj` treats two NuGet-specific warnings as errors (`NU5125`, `NU5048`). +These are packaging concerns and must be preserved in the per-project file (they should not be moved to `Directory.Build.props`). + +## Files affected + +### Created + +- `/workspace/repo/Directory.Build.props` — new file; shared MSBuild properties. + +### Modified + +- `/workspace/repo/HdrHistogram/HdrHistogram.csproj` — remove any properties that become redundant once lifted to `Directory.Build.props`. +- `/workspace/repo/HdrHistogram.UnitTests/HdrHistogram.UnitTests.csproj` — same. +- `/workspace/repo/HdrHistogram.Examples/HdrHistogram.Examples.csproj` — same. +- `/workspace/repo/HdrHistogram.Benchmarking/HdrHistogram.Benchmarking.csproj` — same. + +In practice, because no current per-project settings duplicate what will go in `Directory.Build.props`, the per-project files will only need review (not necessarily editing) unless duplicate entries appear after introducing the new file. + +## Acceptance criteria + +- [ ] `Directory.Build.props` added at repository root. +- [ ] `EnforceCodeStyleInBuild` set to `true` in `Directory.Build.props`. +- [ ] `AnalysisLevel` set to `latest-recommended` in `Directory.Build.props`. +- [ ] `Nullable` set to `enable` in `Directory.Build.props`. +- [ ] `ImplicitUsings` explicitly configured (set to `disable` to match the explicit-using style visible in existing code). +- [ ] `TreatWarningsAsErrors` assessed: start conservative — omit or set per-category at `suggestion` level to avoid breaking the build; a follow-up issue can tighten this. +- [ ] `dotnet build` succeeds with no new errors. +- [ ] `dotnet test` passes with no regressions. +- [ ] Any per-project settings made redundant by `Directory.Build.props` are removed from the individual `.csproj` files. + +## Proposed `Directory.Build.props` content + +```xml + + + + enable + + + disable + + + true + + + latest-recommended + + +``` + +`TreatWarningsAsErrors` is intentionally omitted from the initial implementation. +The `.editorconfig` already uses `suggestion` severity for all style rules, so enabling `EnforceCodeStyleInBuild` alone will not produce errors. +Analyser diagnostics at `latest-recommended` default to warnings; if the build is clean these can be promoted to errors in a follow-up. + +## Test strategy + +No new test code is required. +Verification is build-and-test: + +1. Run `dotnet build` — must exit 0 with no new errors. +2. Run `dotnet test` — all existing tests must pass. +3. Review build output for new warnings introduced by `EnforceCodeStyleInBuild` or `AnalysisLevel`; address or suppress with justified comments if they would block the build. + +If the number of analyser warnings is large, severity for specific rule categories can be downgraded to `suggestion` in `.editorconfig` or via a `` entry in `Directory.Build.props`. + +## Risks and open questions + +| Risk | Likelihood | Mitigation | +|---|---|---| +| Enabling `Nullable` causes widespread nullable warnings/errors on existing code that was written without nullability annotations | High | Nullable warnings default to `warning`, not `error`; the build will not break. Warnings can be addressed incrementally. If the volume is very high, add `annotations` initially (emits fewer warnings) and upgrade in a follow-up. | +| `EnforceCodeStyleInBuild` promotes `.editorconfig` `suggestion`-level rules to build diagnostics | Medium | The `.editorconfig` already sets all rules to `suggestion`; `EnforceCodeStyleInBuild` reports them but at `suggestion` (info) level, which does not fail the build. Verify output after first build. | +| `AnalysisLevel=latest-recommended` introduces new warning rules that fail CI | Medium | Run `dotnet build` locally first; inspect warnings. Use `` or downgrade individual rules via `.editorconfig` if needed. Note that AppVeyor CI does not appear to be configured currently (`appveyor.yml` absent). | +| `ImplicitUsings=disable` conflicts with any project that implicitly relies on them | Low | Existing code has explicit `using` statements; disabling implicit usings is consistent with current practice. | +| `netstandard2.0` target in the main library may not support all analyser rules | Low | Analyser rules apply at the MSBuild/Roslyn layer regardless of target framework; no special handling needed. | + +## Open questions + +1. Should `TreatWarningsAsErrors` be scoped to analyser-only categories (e.g., `$(WarningsAsErrors);CA*`) now, or deferred to a follow-up issue after the warning volume is known? + Recommendation: defer — start conservative per the issue notes. +2. Is there a CI pipeline that needs updating once `EnforceCodeStyleInBuild` is active? + Observation: no `appveyor.yml` found at repo root; CI status is unknown. + Action: verify with the team before tightening rules further. From 6deaed3ac58bc527394f6a9bf24a29666b48eb10 Mon Sep 17 00:00:00 2001 From: leecampbell-codeagent Date: Sun, 1 Mar 2026 14:34:40 +0000 Subject: [PATCH 2/7] plan(#116): review brief --- plan/planning/brief-review.md | 90 +++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 plan/planning/brief-review.md diff --git a/plan/planning/brief-review.md b/plan/planning/brief-review.md new file mode 100644 index 0000000..b66ca54 --- /dev/null +++ b/plan/planning/brief-review.md @@ -0,0 +1,90 @@ +# Brief Review — Issue #116: Add Directory.Build.props + +## Overall assessment + +The brief is clear, well-scoped, and technically accurate in almost every respect. +It can move to ready once the one factual correction below is applied. + +--- + +## Verification results + +All file-existence claims were checked against the repository. + +| Claim in brief | Verified | +|---|---| +| `HdrHistogram.csproj` exists with `TargetFrameworks: net8.0;netstandard2.0` | ✓ | +| `HdrHistogram.UnitTests.csproj` exists with minimal PropertyGroup | ✓ | +| `HdrHistogram.Examples.csproj` exists with `OutputType: Exe` | ✓ | +| `HdrHistogram.Benchmarking.csproj` exists with `OutputType: Exe` | ✓ | +| None of the four projects set `Nullable`, `AnalysisLevel`, `EnforceCodeStyleInBuild`, `ImplicitUsings` | ✓ | +| `HdrHistogram.csproj` has `NU5125;NU5048` | ✓ | +| `.editorconfig` present; all style rules set to `suggestion` severity | ✓ | +| `Directory.Build.props` does not exist | ✓ | +| `Directory.Build.targets`, `global.json`, `nuget.config` do not exist | ✓ | +| `appveyor.yml` does not exist at repo root | ✓ | + +--- + +## Issues requiring correction + +### 1. CI is active — it is not unknown (required fix) + +**Location:** Risk table row for `AnalysisLevel=latest-recommended`, and Open question #2. + +**Current text (risk table):** +> Note that AppVeyor CI does not appear to be configured currently (appveyor.yml absent). + +**Current text (open question #2):** +> Observation: no appveyor.yml found at repo root; CI status is unknown. + +**Actual state:** +CI is active via GitHub Actions at `.github/workflows/ci.yml`. +The pipeline runs on every PR and on pushes to `main`. +It executes: + +``` +dotnet restore +dotnet build -c Release --no-restore /p:Version=... +dotnet test ./HdrHistogram.UnitTests/HdrHistogram.UnitTests.csproj --no-build -c Release +dotnet pack ./HdrHistogram/HdrHistogram.csproj -c Release ... +``` + +`Directory.Build.props` is picked up automatically by MSBuild during the `dotnet build` step, so `EnforceCodeStyleInBuild=true` and `AnalysisLevel=latest-recommended` will be exercised on every PR. + +**Why it matters:** +The implementer needs to know that CI will exercise the new settings automatically. +The mitigation advice ("verify with the team before tightening rules further") should reference the GitHub Actions workflow, not imply the absence of CI. + +**Suggested replacement for the risk table note:** +> CI is active via GitHub Actions (`.github/workflows/ci.yml`). +> The pipeline runs `dotnet build -c Release` and `dotnet test` on every PR. +> New analyser warnings will surface in CI output. +> Because `TreatWarningsAsErrors` is not set, they will not fail the build. + +**Suggested replacement for open question #2:** +> CI runs via GitHub Actions (`.github/workflows/ci.yml`) with `dotnet build -c Release` and `dotnet test`. +> No pipeline changes are needed for this PR; the new settings are exercised automatically. +> If warning volume is high after merging, downgrade specific rule categories via `.editorconfig` in a follow-up. + +--- + +## Minor notes (no action required) + +- `spec/tech-standards/build-system.md` documents an AppVeyor configuration as if it were current. + This is a pre-existing documentation inaccuracy in the spec, not in the brief. + The brief correctly observed that `appveyor.yml` is absent. + Updating the spec doc is out of scope for this issue. + +- The proposed `Directory.Build.props` content is minimal, well-justified, and matches the acceptance criteria exactly. + No changes to the proposed XML are needed. + +- The test strategy (build + test verification, no new test code) is appropriate for a build-system change. + +- Scope is correctly sized for a single PR. + +--- + +## Recommended action + +Apply the CI correction (issue #1 above), then move the brief to `plan/ready/brief.md`. From 90daa94a94dc0edc03b14a8c12af78359f6d438e Mon Sep 17 00:00:00 2001 From: leecampbell-codeagent Date: Sun, 1 Mar 2026 14:35:54 +0000 Subject: [PATCH 3/7] plan(#116): apply brief review feedback --- plan/planning/brief-review.md | 90 ----------------------------------- plan/planning/brief.md | 7 +-- 2 files changed, 4 insertions(+), 93 deletions(-) delete mode 100644 plan/planning/brief-review.md diff --git a/plan/planning/brief-review.md b/plan/planning/brief-review.md deleted file mode 100644 index b66ca54..0000000 --- a/plan/planning/brief-review.md +++ /dev/null @@ -1,90 +0,0 @@ -# Brief Review — Issue #116: Add Directory.Build.props - -## Overall assessment - -The brief is clear, well-scoped, and technically accurate in almost every respect. -It can move to ready once the one factual correction below is applied. - ---- - -## Verification results - -All file-existence claims were checked against the repository. - -| Claim in brief | Verified | -|---|---| -| `HdrHistogram.csproj` exists with `TargetFrameworks: net8.0;netstandard2.0` | ✓ | -| `HdrHistogram.UnitTests.csproj` exists with minimal PropertyGroup | ✓ | -| `HdrHistogram.Examples.csproj` exists with `OutputType: Exe` | ✓ | -| `HdrHistogram.Benchmarking.csproj` exists with `OutputType: Exe` | ✓ | -| None of the four projects set `Nullable`, `AnalysisLevel`, `EnforceCodeStyleInBuild`, `ImplicitUsings` | ✓ | -| `HdrHistogram.csproj` has `NU5125;NU5048` | ✓ | -| `.editorconfig` present; all style rules set to `suggestion` severity | ✓ | -| `Directory.Build.props` does not exist | ✓ | -| `Directory.Build.targets`, `global.json`, `nuget.config` do not exist | ✓ | -| `appveyor.yml` does not exist at repo root | ✓ | - ---- - -## Issues requiring correction - -### 1. CI is active — it is not unknown (required fix) - -**Location:** Risk table row for `AnalysisLevel=latest-recommended`, and Open question #2. - -**Current text (risk table):** -> Note that AppVeyor CI does not appear to be configured currently (appveyor.yml absent). - -**Current text (open question #2):** -> Observation: no appveyor.yml found at repo root; CI status is unknown. - -**Actual state:** -CI is active via GitHub Actions at `.github/workflows/ci.yml`. -The pipeline runs on every PR and on pushes to `main`. -It executes: - -``` -dotnet restore -dotnet build -c Release --no-restore /p:Version=... -dotnet test ./HdrHistogram.UnitTests/HdrHistogram.UnitTests.csproj --no-build -c Release -dotnet pack ./HdrHistogram/HdrHistogram.csproj -c Release ... -``` - -`Directory.Build.props` is picked up automatically by MSBuild during the `dotnet build` step, so `EnforceCodeStyleInBuild=true` and `AnalysisLevel=latest-recommended` will be exercised on every PR. - -**Why it matters:** -The implementer needs to know that CI will exercise the new settings automatically. -The mitigation advice ("verify with the team before tightening rules further") should reference the GitHub Actions workflow, not imply the absence of CI. - -**Suggested replacement for the risk table note:** -> CI is active via GitHub Actions (`.github/workflows/ci.yml`). -> The pipeline runs `dotnet build -c Release` and `dotnet test` on every PR. -> New analyser warnings will surface in CI output. -> Because `TreatWarningsAsErrors` is not set, they will not fail the build. - -**Suggested replacement for open question #2:** -> CI runs via GitHub Actions (`.github/workflows/ci.yml`) with `dotnet build -c Release` and `dotnet test`. -> No pipeline changes are needed for this PR; the new settings are exercised automatically. -> If warning volume is high after merging, downgrade specific rule categories via `.editorconfig` in a follow-up. - ---- - -## Minor notes (no action required) - -- `spec/tech-standards/build-system.md` documents an AppVeyor configuration as if it were current. - This is a pre-existing documentation inaccuracy in the spec, not in the brief. - The brief correctly observed that `appveyor.yml` is absent. - Updating the spec doc is out of scope for this issue. - -- The proposed `Directory.Build.props` content is minimal, well-justified, and matches the acceptance criteria exactly. - No changes to the proposed XML are needed. - -- The test strategy (build + test verification, no new test code) is appropriate for a build-system change. - -- Scope is correctly sized for a single PR. - ---- - -## Recommended action - -Apply the CI correction (issue #1 above), then move the brief to `plan/ready/brief.md`. diff --git a/plan/planning/brief.md b/plan/planning/brief.md index 605ea7c..626893c 100644 --- a/plan/planning/brief.md +++ b/plan/planning/brief.md @@ -107,7 +107,7 @@ If the number of analyser warnings is large, severity for specific rule categori |---|---|---| | Enabling `Nullable` causes widespread nullable warnings/errors on existing code that was written without nullability annotations | High | Nullable warnings default to `warning`, not `error`; the build will not break. Warnings can be addressed incrementally. If the volume is very high, add `annotations` initially (emits fewer warnings) and upgrade in a follow-up. | | `EnforceCodeStyleInBuild` promotes `.editorconfig` `suggestion`-level rules to build diagnostics | Medium | The `.editorconfig` already sets all rules to `suggestion`; `EnforceCodeStyleInBuild` reports them but at `suggestion` (info) level, which does not fail the build. Verify output after first build. | -| `AnalysisLevel=latest-recommended` introduces new warning rules that fail CI | Medium | Run `dotnet build` locally first; inspect warnings. Use `` or downgrade individual rules via `.editorconfig` if needed. Note that AppVeyor CI does not appear to be configured currently (`appveyor.yml` absent). | +| `AnalysisLevel=latest-recommended` introduces new warning rules that fail CI | Medium | Run `dotnet build` locally first; inspect warnings. Use `` or downgrade individual rules via `.editorconfig` if needed. CI is active via GitHub Actions (`.github/workflows/ci.yml`). The pipeline runs `dotnet build -c Release` and `dotnet test` on every PR. New analyser warnings will surface in CI output. Because `TreatWarningsAsErrors` is not set, they will not fail the build. | | `ImplicitUsings=disable` conflicts with any project that implicitly relies on them | Low | Existing code has explicit `using` statements; disabling implicit usings is consistent with current practice. | | `netstandard2.0` target in the main library may not support all analyser rules | Low | Analyser rules apply at the MSBuild/Roslyn layer regardless of target framework; no special handling needed. | @@ -116,5 +116,6 @@ If the number of analyser warnings is large, severity for specific rule categori 1. Should `TreatWarningsAsErrors` be scoped to analyser-only categories (e.g., `$(WarningsAsErrors);CA*`) now, or deferred to a follow-up issue after the warning volume is known? Recommendation: defer — start conservative per the issue notes. 2. Is there a CI pipeline that needs updating once `EnforceCodeStyleInBuild` is active? - Observation: no `appveyor.yml` found at repo root; CI status is unknown. - Action: verify with the team before tightening rules further. + CI runs via GitHub Actions (`.github/workflows/ci.yml`) with `dotnet build -c Release` and `dotnet test`. + No pipeline changes are needed for this PR; the new settings are exercised automatically. + If warning volume is high after merging, downgrade specific rule categories via `.editorconfig` in a follow-up. From 3d0b7112cce72db7f3c1519967b12c7ecb7bce02 Mon Sep 17 00:00:00 2001 From: leecampbell-codeagent Date: Sun, 1 Mar 2026 14:38:34 +0000 Subject: [PATCH 4/7] plan(#116): review brief --- plan/{planning => ready}/brief.md | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename plan/{planning => ready}/brief.md (100%) diff --git a/plan/planning/brief.md b/plan/ready/brief.md similarity index 100% rename from plan/planning/brief.md rename to plan/ready/brief.md From 3b270b065c0330305c481c4fbf88b74e39bd2bdb Mon Sep 17 00:00:00 2001 From: leecampbell-codeagent Date: Sun, 1 Mar 2026 14:41:38 +0000 Subject: [PATCH 5/7] plan(#116): create task breakdown --- plan/ready/task.md | 122 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+) create mode 100644 plan/ready/task.md diff --git a/plan/ready/task.md b/plan/ready/task.md new file mode 100644 index 0000000..72b89f9 --- /dev/null +++ b/plan/ready/task.md @@ -0,0 +1,122 @@ +# Task List — Issue #116: Add Directory.Build.props with shared build settings + +## Context + +No `Directory.Build.props` exists at the repository root. +None of the four projects (`HdrHistogram`, `HdrHistogram.UnitTests`, `HdrHistogram.Examples`, `HdrHistogram.Benchmarking`) define `Nullable`, `ImplicitUsings`, `EnforceCodeStyleInBuild`, `AnalysisLevel`, or `TreatWarningsAsErrors`. +The only per-project `WarningsAsErrors` entry is `NU5125;NU5048` in `HdrHistogram.csproj` — this is NuGet-packaging-specific and must remain in that file. + +No public API changes are made by this issue; XML doc comments do not need updating. +No new tests are required; verification is build-and-test. + +--- + +## Tasks + +### 1 — Create `/workspace/repo/Directory.Build.props` + +- **File**: `/workspace/repo/Directory.Build.props` (new file) +- **Change**: Create with the four shared MSBuild properties from the brief. +- **Why**: Centralises build settings for all four projects. + `TreatWarningsAsErrors` is intentionally omitted — start conservative; a follow-up issue will tighten this once the warning volume is known. +- **Content**: + + ```xml + + + + enable + + + disable + + + true + + + latest-recommended + + + ``` + +- **Verify**: File exists at `/workspace/repo/Directory.Build.props` with all four properties set to the values above and `TreatWarningsAsErrors` absent. + +--- + +### 2 — Review `HdrHistogram/HdrHistogram.csproj` for redundant properties + +- **File**: `/workspace/repo/HdrHistogram/HdrHistogram.csproj` +- **Change**: Audit all `PropertyGroup` entries. + Confirm none of `Nullable`, `ImplicitUsings`, `EnforceCodeStyleInBuild`, or `AnalysisLevel` are present (they are not, per exploration). + Confirm `WarningsAsErrors>NU5125;NU5048` is retained — it is NuGet-specific and must not move to `Directory.Build.props`. +- **Why**: Per acceptance criterion AC9, any properties made redundant by `Directory.Build.props` must be removed. + The exploration confirmed no redundant properties exist in this file, but the audit must be on-record. +- **Verify**: File is unchanged (or has only redundant properties removed if any were found during audit); `NU5125;NU5048` entry is still present. + +--- + +### 3 — Review `HdrHistogram.UnitTests/HdrHistogram.UnitTests.csproj` for redundant properties + +- **File**: `/workspace/repo/HdrHistogram.UnitTests/HdrHistogram.UnitTests.csproj` +- **Change**: Audit all `PropertyGroup` entries. + The exploration shows only `TargetFrameworks` is present — no overlap with `Directory.Build.props` properties. +- **Why**: AC9 compliance audit. +- **Verify**: File is unchanged (or has only redundant properties removed if any were found during audit). + +--- + +### 4 — Review `HdrHistogram.Examples/HdrHistogram.Examples.csproj` for redundant properties + +- **File**: `/workspace/repo/HdrHistogram.Examples/HdrHistogram.Examples.csproj` +- **Change**: Audit all `PropertyGroup` entries. + The exploration shows only `OutputType` and `TargetFrameworks` — no overlap with `Directory.Build.props` properties. +- **Why**: AC9 compliance audit. +- **Verify**: File is unchanged (or has only redundant properties removed if any were found during audit). + +--- + +### 5 — Review `HdrHistogram.Benchmarking/HdrHistogram.Benchmarking.csproj` for redundant properties + +- **File**: `/workspace/repo/HdrHistogram.Benchmarking/HdrHistogram.Benchmarking.csproj` +- **Change**: Audit all `PropertyGroup` entries. + The exploration shows only `OutputType` and `TargetFrameworks` — no overlap with `Directory.Build.props` properties. +- **Why**: AC9 compliance audit. +- **Verify**: File is unchanged (or has only redundant properties removed if any were found during audit). + +--- + +### 6 — Run `dotnet build` and address any new diagnostics + +- **Command**: `dotnet build -c Release` from `/workspace/repo` +- **Change**: If new build errors are introduced (unexpected), investigate and resolve. + If the volume of new analyser warnings is large, add targeted `` entries or downgrade specific rule categories in `.editorconfig` — document the reason with an inline comment. + If `EnforceCodeStyleInBuild` or `AnalysisLevel` produce warnings that would break CI, suppress them with justification. + `TreatWarningsAsErrors` is not set, so warnings alone will not fail the build. +- **Why**: AC7 — `dotnet build` must exit 0 with no new errors. +- **Verify**: `dotnet build -c Release` exits with code 0 and produces no new errors (warnings are acceptable). + +--- + +### 7 — Run `dotnet test` and confirm no regressions + +- **Command**: `dotnet test ./HdrHistogram.UnitTests/HdrHistogram.UnitTests.csproj -c Release` from `/workspace/repo` +- **Change**: None expected. + If any test fails due to a build-configuration side-effect, investigate and fix. +- **Why**: AC8 — all existing tests must pass. +- **Verify**: `dotnet test` exits with code 0; all 32 source files worth of tests pass with no failures or skips beyond the pre-existing baseline. + +--- + +## Acceptance Criteria Cross-Reference + +| Criterion | Covered by | +|---|---| +| AC1 — `Directory.Build.props` added at repository root | Task 1 | +| AC2 — `EnforceCodeStyleInBuild` set to `true` | Task 1 | +| AC3 — `AnalysisLevel` set to `latest-recommended` | Task 1 | +| AC4 — `Nullable` set to `enable` | Task 1 | +| AC5 — `ImplicitUsings` set to `disable` | Task 1 | +| AC6 — `TreatWarningsAsErrors` assessed (omitted conservatively) | Task 1 | +| AC7 — `dotnet build` succeeds with no new errors | Task 6 | +| AC8 — `dotnet test` passes with no regressions | Task 7 | +| AC9 — Redundant per-project settings removed | Tasks 2–5 | From 843d4eaa53ffd0105c4e6f44e9c02a4c0ea34eab Mon Sep 17 00:00:00 2001 From: leecampbell-codeagent Date: Sun, 1 Mar 2026 14:49:44 +0000 Subject: [PATCH 6/7] feat(#116): implement tasks --- Directory.Build.props | 21 +++++++++++++++++++++ plan/{ready => done}/brief.md | 0 plan/{ready => done}/task.md | 0 3 files changed, 21 insertions(+) create mode 100644 Directory.Build.props rename plan/{ready => done}/brief.md (100%) rename plan/{ready => done}/task.md (100%) diff --git a/Directory.Build.props b/Directory.Build.props new file mode 100644 index 0000000..737b768 --- /dev/null +++ b/Directory.Build.props @@ -0,0 +1,21 @@ + + + + enable + + + disable + + + true + + + latest-recommended + + + + + 8.0 + + diff --git a/plan/ready/brief.md b/plan/done/brief.md similarity index 100% rename from plan/ready/brief.md rename to plan/done/brief.md diff --git a/plan/ready/task.md b/plan/done/task.md similarity index 100% rename from plan/ready/task.md rename to plan/done/task.md From 58086d099ac75ef475e8a2e35b74531a22ce3a89 Mon Sep 17 00:00:00 2001 From: leecampbell-codeagent Date: Sun, 1 Mar 2026 14:50:16 +0000 Subject: [PATCH 7/7] feat(#116): complete implementation --- plan/done/brief.md | 121 -------------------------------------------- plan/done/task.md | 122 --------------------------------------------- 2 files changed, 243 deletions(-) delete mode 100644 plan/done/brief.md delete mode 100644 plan/done/task.md diff --git a/plan/done/brief.md b/plan/done/brief.md deleted file mode 100644 index 626893c..0000000 --- a/plan/done/brief.md +++ /dev/null @@ -1,121 +0,0 @@ -# Issue #116 — Add Directory.Build.props with shared build settings - -## Summary - -The repository currently has no centralised MSBuild property file. -Each of the four projects (`HdrHistogram`, `HdrHistogram.UnitTests`, `HdrHistogram.Examples`, `HdrHistogram.Benchmarking`) carries only minimal PropertyGroup settings (`TargetFramework(s)`, `OutputType`). -None of the projects configure `Nullable`, `ImplicitUsings`, `EnforceCodeStyleInBuild`, `AnalysisLevel`, or `TreatWarningsAsErrors`. - -Adding a `Directory.Build.props` file at the repository root will: - -- Centralise these settings in one place. -- Enforce `.editorconfig` style rules during build. -- Enable the built-in .NET analysers at the `latest-recommended` level. -- Establish a consistent nullable-reference-types policy across all projects. -- Avoid duplication as new projects are added. - -## Current state (confirmed by exploration) - -### Projects - -| Project | TargetFramework(s) | OutputType | Nullable | AnalysisLevel | EnforceCodeStyleInBuild | TreatWarningsAsErrors | -|---|---|---|---|---|---|---| -| HdrHistogram | net8.0;netstandard2.0 | (library) | — | — | — | NU5125;NU5048 only | -| HdrHistogram.UnitTests | net8.0 | — | — | — | — | — | -| HdrHistogram.Examples | net8.0 | Exe | — | — | — | — | -| HdrHistogram.Benchmarking | net8.0 | Exe | — | — | — | — | - -### Root configuration - -- `.editorconfig` — present, comprehensive; all code-style rules are set to `suggestion` severity. -- `Directory.Build.props` — **does not exist** (to be created by this issue). -- `Directory.Build.targets` — does not exist. -- `global.json` — does not exist. -- `nuget.config` — does not exist. -- `appveyor.yml` — does not exist. - -### Existing per-project WarningsAsErrors - -`HdrHistogram.csproj` treats two NuGet-specific warnings as errors (`NU5125`, `NU5048`). -These are packaging concerns and must be preserved in the per-project file (they should not be moved to `Directory.Build.props`). - -## Files affected - -### Created - -- `/workspace/repo/Directory.Build.props` — new file; shared MSBuild properties. - -### Modified - -- `/workspace/repo/HdrHistogram/HdrHistogram.csproj` — remove any properties that become redundant once lifted to `Directory.Build.props`. -- `/workspace/repo/HdrHistogram.UnitTests/HdrHistogram.UnitTests.csproj` — same. -- `/workspace/repo/HdrHistogram.Examples/HdrHistogram.Examples.csproj` — same. -- `/workspace/repo/HdrHistogram.Benchmarking/HdrHistogram.Benchmarking.csproj` — same. - -In practice, because no current per-project settings duplicate what will go in `Directory.Build.props`, the per-project files will only need review (not necessarily editing) unless duplicate entries appear after introducing the new file. - -## Acceptance criteria - -- [ ] `Directory.Build.props` added at repository root. -- [ ] `EnforceCodeStyleInBuild` set to `true` in `Directory.Build.props`. -- [ ] `AnalysisLevel` set to `latest-recommended` in `Directory.Build.props`. -- [ ] `Nullable` set to `enable` in `Directory.Build.props`. -- [ ] `ImplicitUsings` explicitly configured (set to `disable` to match the explicit-using style visible in existing code). -- [ ] `TreatWarningsAsErrors` assessed: start conservative — omit or set per-category at `suggestion` level to avoid breaking the build; a follow-up issue can tighten this. -- [ ] `dotnet build` succeeds with no new errors. -- [ ] `dotnet test` passes with no regressions. -- [ ] Any per-project settings made redundant by `Directory.Build.props` are removed from the individual `.csproj` files. - -## Proposed `Directory.Build.props` content - -```xml - - - - enable - - - disable - - - true - - - latest-recommended - - -``` - -`TreatWarningsAsErrors` is intentionally omitted from the initial implementation. -The `.editorconfig` already uses `suggestion` severity for all style rules, so enabling `EnforceCodeStyleInBuild` alone will not produce errors. -Analyser diagnostics at `latest-recommended` default to warnings; if the build is clean these can be promoted to errors in a follow-up. - -## Test strategy - -No new test code is required. -Verification is build-and-test: - -1. Run `dotnet build` — must exit 0 with no new errors. -2. Run `dotnet test` — all existing tests must pass. -3. Review build output for new warnings introduced by `EnforceCodeStyleInBuild` or `AnalysisLevel`; address or suppress with justified comments if they would block the build. - -If the number of analyser warnings is large, severity for specific rule categories can be downgraded to `suggestion` in `.editorconfig` or via a `` entry in `Directory.Build.props`. - -## Risks and open questions - -| Risk | Likelihood | Mitigation | -|---|---|---| -| Enabling `Nullable` causes widespread nullable warnings/errors on existing code that was written without nullability annotations | High | Nullable warnings default to `warning`, not `error`; the build will not break. Warnings can be addressed incrementally. If the volume is very high, add `annotations` initially (emits fewer warnings) and upgrade in a follow-up. | -| `EnforceCodeStyleInBuild` promotes `.editorconfig` `suggestion`-level rules to build diagnostics | Medium | The `.editorconfig` already sets all rules to `suggestion`; `EnforceCodeStyleInBuild` reports them but at `suggestion` (info) level, which does not fail the build. Verify output after first build. | -| `AnalysisLevel=latest-recommended` introduces new warning rules that fail CI | Medium | Run `dotnet build` locally first; inspect warnings. Use `` or downgrade individual rules via `.editorconfig` if needed. CI is active via GitHub Actions (`.github/workflows/ci.yml`). The pipeline runs `dotnet build -c Release` and `dotnet test` on every PR. New analyser warnings will surface in CI output. Because `TreatWarningsAsErrors` is not set, they will not fail the build. | -| `ImplicitUsings=disable` conflicts with any project that implicitly relies on them | Low | Existing code has explicit `using` statements; disabling implicit usings is consistent with current practice. | -| `netstandard2.0` target in the main library may not support all analyser rules | Low | Analyser rules apply at the MSBuild/Roslyn layer regardless of target framework; no special handling needed. | - -## Open questions - -1. Should `TreatWarningsAsErrors` be scoped to analyser-only categories (e.g., `$(WarningsAsErrors);CA*`) now, or deferred to a follow-up issue after the warning volume is known? - Recommendation: defer — start conservative per the issue notes. -2. Is there a CI pipeline that needs updating once `EnforceCodeStyleInBuild` is active? - CI runs via GitHub Actions (`.github/workflows/ci.yml`) with `dotnet build -c Release` and `dotnet test`. - No pipeline changes are needed for this PR; the new settings are exercised automatically. - If warning volume is high after merging, downgrade specific rule categories via `.editorconfig` in a follow-up. diff --git a/plan/done/task.md b/plan/done/task.md deleted file mode 100644 index 72b89f9..0000000 --- a/plan/done/task.md +++ /dev/null @@ -1,122 +0,0 @@ -# Task List — Issue #116: Add Directory.Build.props with shared build settings - -## Context - -No `Directory.Build.props` exists at the repository root. -None of the four projects (`HdrHistogram`, `HdrHistogram.UnitTests`, `HdrHistogram.Examples`, `HdrHistogram.Benchmarking`) define `Nullable`, `ImplicitUsings`, `EnforceCodeStyleInBuild`, `AnalysisLevel`, or `TreatWarningsAsErrors`. -The only per-project `WarningsAsErrors` entry is `NU5125;NU5048` in `HdrHistogram.csproj` — this is NuGet-packaging-specific and must remain in that file. - -No public API changes are made by this issue; XML doc comments do not need updating. -No new tests are required; verification is build-and-test. - ---- - -## Tasks - -### 1 — Create `/workspace/repo/Directory.Build.props` - -- **File**: `/workspace/repo/Directory.Build.props` (new file) -- **Change**: Create with the four shared MSBuild properties from the brief. -- **Why**: Centralises build settings for all four projects. - `TreatWarningsAsErrors` is intentionally omitted — start conservative; a follow-up issue will tighten this once the warning volume is known. -- **Content**: - - ```xml - - - - enable - - - disable - - - true - - - latest-recommended - - - ``` - -- **Verify**: File exists at `/workspace/repo/Directory.Build.props` with all four properties set to the values above and `TreatWarningsAsErrors` absent. - ---- - -### 2 — Review `HdrHistogram/HdrHistogram.csproj` for redundant properties - -- **File**: `/workspace/repo/HdrHistogram/HdrHistogram.csproj` -- **Change**: Audit all `PropertyGroup` entries. - Confirm none of `Nullable`, `ImplicitUsings`, `EnforceCodeStyleInBuild`, or `AnalysisLevel` are present (they are not, per exploration). - Confirm `WarningsAsErrors>NU5125;NU5048` is retained — it is NuGet-specific and must not move to `Directory.Build.props`. -- **Why**: Per acceptance criterion AC9, any properties made redundant by `Directory.Build.props` must be removed. - The exploration confirmed no redundant properties exist in this file, but the audit must be on-record. -- **Verify**: File is unchanged (or has only redundant properties removed if any were found during audit); `NU5125;NU5048` entry is still present. - ---- - -### 3 — Review `HdrHistogram.UnitTests/HdrHistogram.UnitTests.csproj` for redundant properties - -- **File**: `/workspace/repo/HdrHistogram.UnitTests/HdrHistogram.UnitTests.csproj` -- **Change**: Audit all `PropertyGroup` entries. - The exploration shows only `TargetFrameworks` is present — no overlap with `Directory.Build.props` properties. -- **Why**: AC9 compliance audit. -- **Verify**: File is unchanged (or has only redundant properties removed if any were found during audit). - ---- - -### 4 — Review `HdrHistogram.Examples/HdrHistogram.Examples.csproj` for redundant properties - -- **File**: `/workspace/repo/HdrHistogram.Examples/HdrHistogram.Examples.csproj` -- **Change**: Audit all `PropertyGroup` entries. - The exploration shows only `OutputType` and `TargetFrameworks` — no overlap with `Directory.Build.props` properties. -- **Why**: AC9 compliance audit. -- **Verify**: File is unchanged (or has only redundant properties removed if any were found during audit). - ---- - -### 5 — Review `HdrHistogram.Benchmarking/HdrHistogram.Benchmarking.csproj` for redundant properties - -- **File**: `/workspace/repo/HdrHistogram.Benchmarking/HdrHistogram.Benchmarking.csproj` -- **Change**: Audit all `PropertyGroup` entries. - The exploration shows only `OutputType` and `TargetFrameworks` — no overlap with `Directory.Build.props` properties. -- **Why**: AC9 compliance audit. -- **Verify**: File is unchanged (or has only redundant properties removed if any were found during audit). - ---- - -### 6 — Run `dotnet build` and address any new diagnostics - -- **Command**: `dotnet build -c Release` from `/workspace/repo` -- **Change**: If new build errors are introduced (unexpected), investigate and resolve. - If the volume of new analyser warnings is large, add targeted `` entries or downgrade specific rule categories in `.editorconfig` — document the reason with an inline comment. - If `EnforceCodeStyleInBuild` or `AnalysisLevel` produce warnings that would break CI, suppress them with justification. - `TreatWarningsAsErrors` is not set, so warnings alone will not fail the build. -- **Why**: AC7 — `dotnet build` must exit 0 with no new errors. -- **Verify**: `dotnet build -c Release` exits with code 0 and produces no new errors (warnings are acceptable). - ---- - -### 7 — Run `dotnet test` and confirm no regressions - -- **Command**: `dotnet test ./HdrHistogram.UnitTests/HdrHistogram.UnitTests.csproj -c Release` from `/workspace/repo` -- **Change**: None expected. - If any test fails due to a build-configuration side-effect, investigate and fix. -- **Why**: AC8 — all existing tests must pass. -- **Verify**: `dotnet test` exits with code 0; all 32 source files worth of tests pass with no failures or skips beyond the pre-existing baseline. - ---- - -## Acceptance Criteria Cross-Reference - -| Criterion | Covered by | -|---|---| -| AC1 — `Directory.Build.props` added at repository root | Task 1 | -| AC2 — `EnforceCodeStyleInBuild` set to `true` | Task 1 | -| AC3 — `AnalysisLevel` set to `latest-recommended` | Task 1 | -| AC4 — `Nullable` set to `enable` | Task 1 | -| AC5 — `ImplicitUsings` set to `disable` | Task 1 | -| AC6 — `TreatWarningsAsErrors` assessed (omitted conservatively) | Task 1 | -| AC7 — `dotnet build` succeeds with no new errors | Task 6 | -| AC8 — `dotnet test` passes with no regressions | Task 7 | -| AC9 — Redundant per-project settings removed | Tasks 2–5 |