Skip to content

Improve UnitsNet.Modular samples - #1722

Merged
angularsen merged 5 commits into
masterfrom
agl-codex/improve-modular-samples
Aug 2, 2026
Merged

Improve UnitsNet.Modular samples#1722
angularsen merged 5 commits into
masterfrom
agl-codex/improve-modular-samples

Conversation

@angularsen

Copy link
Copy Markdown
Owner

Summary

  • reduce the UnitsNet.Modular sample portfolio to six purpose-focused scenarios with concise names
  • replace ConsumerOwned with a documented, solution-grouped SharedUnitsLibrarySample
  • let every sample switch between ProjectReferences, LocalPackages, and PublishedPackages through the IDE solution-platform selector
  • move the runnable compatibility pair into test fixtures and update documentation, CI, VS Code, and devcontainer paths

Motivation

The previous samples mixed dependency sources into separate projects, used long or ambiguous names, and contained overlapping scenarios. This made it difficult to tell why each sample existed or to compare the same sample against source, locally packed, and published dependencies.

The new solution platforms keep Debug and Release as build configurations while treating the dependency source as the IDE-selectable platform. Local package preparation runs once at solution level to avoid parallel pack races.

Impact

In Rider or Visual Studio, contributors can select combinations such as Debug | LocalPackages for the complete sample portfolio. Direct sample builds continue to default to project references. Outputs and restore state are isolated by dependency platform.

Validation

  • dotnet build UnitsNet.Modular.slnx --configuration Release -p:Platform=ProjectReferences
  • dotnet build UnitsNet.Modular.slnx --configuration Release -p:Platform=LocalPackages
  • dotnet build UnitsNet.Modular.slnx --configuration Release -p:Platform=PublishedPackages
  • dotnet test UnitsNet.Modular.slnx --configuration Release -p:Platform=ProjectReferences — 116 tests passed
  • local-package custom and shared-units samples executed successfully
  • published-package getting-started sample executed successfully
  • git diff --check

@angularsen
angularsen force-pushed the agl-codex/improve-modular-samples branch from a13c16c to a8c3211 Compare August 1, 2026 23:00
@angularsen
angularsen force-pushed the agl-codex/improve-modular-samples branch from a8c3211 to a52e676 Compare August 1, 2026 23:03
@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

⚠️ Review: Non-blocking findings

✅ Breaking changes

  • ✅ No breaking changes to the public UnitsNet API — this PR only touches UnitsNet.Modular samples, its solution/build infrastructure, docs, and CI. Nothing under UnitsNet/GeneratedCode, Common/UnitDefinitions, or UnitsNet/CustomCode is touched.
  • UnitsNet.Modular itself is still pre-1.0 (6.0.0-alpha.2), so renaming/removing/merging sample projects and namespaces (e.g. UnitsNet.Modular.PlaygroundUnitsNet.Modular.Samples.Playground) carries no compatibility risk for consumers.

✅ New quantities or units

  • ✅ Not applicable — no files in Common/UnitDefinitions/ are added or modified, so the criteria in .agents/criteria-for-adding-quantities-and-units.md don't apply here.

✅ Changes to generated code

  • ✅ Not applicable — no generator (CodeGen/), generated (*/GeneratedCode/), or JSON definition files are touched, so there's nothing to check against Length/Temperature/Level-style generated output.

⚠️ Code quality & best practices

  • ✅ The Directory.Solution.targets fix directly addresses the parallel-dotnet pack race called out in the PR description: bootstrapping the shared local feed once (BuildInParallel="false") before restore, plus switching the pack stamp from a per-consuming-project name to a single shared .UnitsNet.Modular.local-pack.stamp, and splitting _PackLocalUnitsNetModularPackage / _PackAdditionalLocalUnitsNetModularPackages into separate incremental targets so a definitions-only change doesn't force a runtime repack.
  • ⚠️ The solution-level bootstrap in Directory.Solution.targets only runs _PackLocalUnitsNetModularPackages through SharedUnitsLibrarySample.Units.csproj. This works today because the runtime stamp is now shared/global, but it only fires for .slnx-driven restores. If a contributor runs two LocalPackages sample builds directly and concurrently (e.g. dotnet build ProjectA.csproj and dotnet build ProjectB.csproj outside the solution restore), the original race could still resurface since only the solution build gets the serialized bootstrap. Worth a short note in ARCHITECTURE.md if that gap is accepted.
  • ⚠️ The new "Run published-package consumer" CI step (.github/workflows/unitsnet-modular-ci.yml) pins UnitsNetModularPublishedVersion to 6.0.0-alpha.2 from nuget.org via UnitsNet.Modular/Samples/Directory.Build.props. This adds a live network dependency and a fixed prerelease version to the regular CI path — if that version is ever unlisted or nuget.org has a hiccup, the job fails even though nothing in the PR changed. Worth confirming this tradeoff is intentional (vs. making it a scheduled/best-effort job).

✅ Test coverage

  • ✅ The compatibility-test fixture reorg (UnitsNet.Modular.Compatibility.Tests/Fixtures/{Generated,Legacy,Shared}) with alias-based ProjectReferences and Compile Remove="Fixtures/**/*.cs" cleanly separates the two aliased scenario assemblies from the test project itself, avoiding ambiguous-type issues while preserving the existing compatibility comparisons.
  • ✅ The new "Run published-package consumer" CI step adds coverage for the previously-untested PublishedPackages platform, complementing the existing ProjectReferences and LocalPackages coverage mentioned in the PR's validation section.

✅ Security concerns

  • ✅ No concerns identified — no new external/untrusted input handling, and the only new network access (restoring from nuget.org in PublishedPackages mode) is a standard NuGet restore, not user-controlled input.

@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

⚠️ Review: Non-blocking findings

This PR only touches the UnitsNet.Modular sample portfolio, build plumbing, CI, and docs. It does not touch Common/UnitDefinitions/*.json, CodeGen/, or UnitsNet/GeneratedCode/, so the "new quantities/units" and "generated code" review criteria don't apply here.

✅ Breaking changes

  • ✅ No changes to the public UnitsNet library API, generated quantity types, or unit definitions — this is entirely UnitsNet.Modular sample/build tooling.
  • ✅ Renamed sample namespaces (UnitsNet.Modular.PlaygroundUnitsNet.Modular.Samples.Playground, etc.) are internal to sample console apps and have no external consumer impact.

✅ New quantities or units

  • ✅ Not applicable — no JSON unit definitions were added or modified.

✅ Generated code

  • ✅ Not applicable — no changes to CodeGen/ or Common/UnitDefinitions/; UnitsNet/GeneratedCode/*.g.cs is untouched.

⚠️ Code quality & correctness

  • ✅ Good DRY cleanup: centralizing ProjectReference/analyzer wiring, CompilerGeneratedFilesOutputPath, and local-package plumbing into Samples/Directory.Build.props/.targets removes a lot of duplicated boilerplate that existed across the old per-sample .csproj files.
  • ✅ Repurposing the solution Platform axis (ProjectReferences/LocalPackages/PublishedPackages) instead of ad hoc MSBuild properties is a reasonable way to let Rider/VS switch dependency source via the IDE selector, and it's well explained in Samples/README.md and ARCHITECTURE.md.
  • ⚠️ UnitsNet.Modular.LocalPackages.targets: UnitsNetModularSampleUpdateLocalPackagesOnBuild now defaults to true whenever LocalPackages is selected, regardless of configuration (previously it defaulted on for Debug only and off otherwise). That means a Release | LocalPackages build will now always re-pack/refresh local packages unless a contributor explicitly passes =false. Worth confirming this is intentional, since it changes the previous default behavior for non-Debug configurations.
  • ⚠️ The new "Run published-package consumer" CI step builds GettingStartedSample against -p:Platform=PublishedPackages, which resolves UnitsNet.Modular 6.0.0-alpha.2 (hardcoded default in Directory.Packages.props) from nuget.org. I couldn't verify from the repo alone whether that prerelease version is actually published — if it isn't (or is later unlisted), this CI job will fail. Worth double-checking before merge, and consider whether this version should be pinned somewhere more visible/updatable (e.g. a CI env var) so it doesn't silently rot.

✅ Test coverage

  • ✅ Compatibility test fixtures were moved under UnitsNet.Modular.Compatibility.Tests/Fixtures/{Generated,Legacy,Shared} with project reference paths correctly updated (verified no dangling references to old sample paths remain anywhere in .md/.yml/.json/.targets/.props/.slnx files).
  • ✅ Test logic itself is unchanged aside from a rename (GeneratedRegistry_DescribesAndOperatesOnTheConsumerOwnedCatalog...OnTheSelectedCatalog) — no coverage regression.

✅ Security

  • ✅ No security concerns — this is internal sample/build tooling with no new input handling or secrets.

Overall this is a well-documented, carefully-executed restructuring. The two ⚠️ items above are worth a quick confirmation from the author but shouldn't block merge if the published-package version is confirmed live and the Release+LocalPackages default is intentional.

@codecov

codecov Bot commented Aug 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98%. Comparing base (c8aa7c1) to head (b2dfcc0).

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1722   +/-   ##
======================================
  Coverage      98%     98%           
======================================
  Files         514     514           
  Lines       24141   24141           
======================================
  Hits        23748   23748           
  Misses        393     393           
Flag Coverage Δ
net48 97% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review outcome: Non-blocking findings ⚠️

This PR only touches the experimental/incubating UnitsNet.Modular sub-project (samples, solution/MSBuild plumbing, CI, and compatibility-test fixtures). It does not touch Common/UnitDefinitions/, CodeGen/, or UnitsNet/GeneratedCode/, so the notes on generated code and new quantities/units are not applicable here.

Breaking changes

  • ✅ No changes to the public UnitsNet package API, generated code, or unit definitions — nothing breaking for existing library consumers.
  • ✅ Sample project/namespace renames (e.g. ConsumerOwned.* to SharedUnitsLibrarySample.*, UnitsNet.Modular.Playground to UnitsNet.Modular.Samples.Playground) are confined to non-shipped sample code, so they do not affect the UnitsNet.Modular package surface itself.

New quantities or units

  • ✅ None added — not applicable to this PR.

Changes to generated code

  • ✅ None — CodeGen and */GeneratedCode/ are untouched, so there is nothing to evaluate for Length/Temperature/Level-style quantities here.

Code quality and MSBuild conventions ⚠️

  • ✅ The incremental "local pack" MSBuild target was correctly split into _PackLocalUnitsNetModularPackage (main package) and _PackAdditionalLocalUnitsNetModularPackages (definition package), each with its own Inputs/Outputs stamp. This is a real improvement over the old combined target, which kept one stamp file per consuming project name (.$(MSBuildProjectName).local-pack.stamp), meaning every sample that pulled in the main package redundantly repacked it under its own stamp. The new shared .UnitsNet.Modular.local-pack.stamp avoids that duplication.
  • ✅ Restricting RestoreSources per dependency mode (local feed only for LocalPackages, https://api.nuget.org/v3/index.json only for PublishedPackages) is a nice touch for reproducibility and avoids accidental dependency-confusion between modes.
  • ⚠️ The new root-level Directory.Solution.targets is placed at the repository root rather than alongside the UnitsNet.Modular.slnx tree — this is required since that .slnx also lives at the repo root, but it is easy to miss that this file also gets evaluated for solution restores of the main UnitsNet.slnx. It is harmless today since it is gated on Platform == LocalPackages (which the main solution never sets), but a one-line comment noting it is scoped to Modular would help a future reader.
  • ⚠️ In UnitsNet.Modular.slnx, sample projects get explicit identity platform mappings (e.g. Platform Solution="*|LocalPackages" Project="LocalPackages") while the core projects (UnitsNet.Modular.csproj, generator, tests) rely on implicit default mapping despite also declaring the same three Platforms. Worth double-checking in Rider/VS that switching the solution platform does not leave the core projects on a stale/default platform — if identity mapping is truly automatic there was no need to spell it out for the samples either, so the asymmetry is worth a quick comment or verification.

Potential bugs

  • ✅ No correctness issues found in the refactored pack targets — since consumers restore against a floating wildcard version (6.0.0-local.dev.*) per package ID, the two packages (UnitsNet.Modular and the definitions package) do not need matching timestamps even though they now pack independently.

Performance

  • ✅ No concerns identified; the split incremental targets should mean less redundant dotnet pack invocation across samples, not more.

Test coverage ⚠️

  • ✅ Good addition: a new "Run published-package consumer" CI step actually exercises the PublishedPackages platform end-to-end (previously only LocalPackages/ProjectReferences were exercised in CI for the getting-started sample).
  • ⚠️ That new step pins UnitsNetModularPublishedVersion to 6.0.0-alpha.2 by default. Please confirm that version is already published on NuGet.org before merging — if not, this CI step will fail until it is.
  • ⚠️ The "custom-definition NuGet consumer" CI step switched from the old run.ps1 (which used an isolated RestorePackagesPath) to a plain dotnet run -p:Platform=LocalPackages. On ephemeral GitHub Actions runners this is low risk, but if this step is ever run on a persistent self-hosted runner or reused locally, losing the isolated package cache could let a stale/global NuGet cache mask a broken local package. Worth a quick sanity check that this is intentional.

Security concerns

  • ✅ No medium+ severity issues identified. Restricting restore sources per platform (see above) is actually a positive security practice.

@angularsen
angularsen marked this pull request as ready for review August 1, 2026 23:21
@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

⚠️ Review: Non-blocking findings

Scope note: this PR only touches UnitsNet.Modular (a separate preview module, currently 6.0.0-alpha.*) — samples, MSBuild plumbing, docs, CI, devcontainer/VS Code config, and test-fixture reorganization. No changes to Common/UnitDefinitions/, CodeGen/, or UnitsNet/GeneratedCode/.

✅ Breaking changes

  • ✅ No changes to the main UnitsNet library's public API. All churn is inside UnitsNet.Modular samples/tooling, which is an unreleased preview module, so there's no impact on published UnitsNet consumers.

✅ New quantities or units

  • ✅ N/A — no quantities or units were added or modified in this PR.

✅ Generated code changes

  • ✅ N/A — no changes under CodeGen/, Common/UnitDefinitions/, or */GeneratedCode/. UnitsNet.Modular's source generator itself is untouched; only sample projects that invoke it were restructured.

⚠️ Code quality & correctness

  • ⚠️ UnitsNet.Modular/Samples/Directory.Build.props sets OutputPath to Artifacts\$(MSBuildProjectName)\$(UnitsNetModularSampleDependencyMode) without including $(Configuration) or $(TargetFramework). Building the same sample+platform combo under both Debug and Release (or across TFMs, if a sample ever multi-targets) will write to the same output folder and can leave stale/mixed-configuration binaries since dotnet build doesn't clean by default. Consider appending \$(Configuration)\$(TargetFramework) to keep parity with the SDK default.
  • ⚠️ UnitsNet.Modular/Samples/UnitsNet.Modular.LocalPackages.targets now has both _PackLocalUnitsNetModularPackage (singular, packs the runtime) and _PackLocalUnitsNetModularPackages (plural, aggregator target that depends on the singular + _PackAdditionalLocalUnitsNetModularPackages). The near-identical names are easy to mix up when this file is touched again — a more distinct name for one of them would reduce the risk of a future typo silently skipping a pack step.
  • ⚠️ Directory.Solution.targets hardcodes the bootstrap project as UnitsNet.Modular/Samples/SharedUnitsLibrarySample/SharedUnitsLibrarySample.Units/SharedUnitsLibrarySample.Units.csproj to pre-pack the shared local-feed package before solution restore. If that project is ever renamed/removed independently of this target, the solution-level race-avoidance silently stops working (each sample would fall back to its own per-project pack, reintroducing the parallel-dotnet pack race this file exists to prevent) rather than failing loudly.
  • ⚠️ UnitsNet.Modular/Samples/GettingStartedSample/run.ps1 dropped -p:ImportDirectoryBuildProps=false -p:ImportDirectoryBuildTargets=false, which previously made this sample build without any repository Directory.Build.* imports to closely emulate a real external consumer. Now it always picks up the repo's sample-infrastructure props/targets. This looks like an intentional tradeoff to let one project support all three platforms, but it does reduce this sample's fidelity as a "true external consumer" smoke test — worth confirming that's acceptable.

⚠️ CI / tooling

  • ⚠️ The new "Run published-package consumer" CI step (.github/workflows/unitsnet-modular-ci.yml) builds GettingStartedSample with -p:Platform=PublishedPackages, which restores UnitsNet.Modular from nuget.org (pinned to 6.0.0-alpha.2 via Directory.Packages.props). This adds a real external-network dependency to CI and will break the pipeline if that specific prerelease is ever unlisted/deleted from NuGet — consider whether this should be a best-effort/non-blocking step or otherwise resilient to that.

✅ Test coverage

  • UnitsNet.Modular.Compatibility.Tests fixtures were cleanly moved under Fixtures/Generated and Fixtures/Legacy with only path/alias updates (CompatibilityTests.cs, .csproj references) — behavior and coverage look unchanged, and the PR description reports 116 tests passing under ProjectReferences.

✅ Security concerns

  • ✅ No security concerns identified (no secrets, no unsafe restore sources — LocalPackages is scoped to the repo-local feed, PublishedPackages uses the standard public NuGet feed).

@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

⚠️ Review: Non-blocking findings

This PR is a pure UnitsNet.Modular sample/build-infrastructure reorganization (project renames, a new Platform (ProjectReferences/LocalPackages/PublishedPackages) selector, MSBuild Directory.Build.* restructuring, CI/devcontainer/VS Code path updates). It does not touch Common/UnitDefinitions/*, GeneratedCode/*, or any shipped UnitsNet public API, so most of the standard checklist doesn't apply here.

✅ Breaking changes

  • ✅ No changes to Common/UnitDefinitions/* or */GeneratedCode/* — no impact on the public UnitsNet / UnitsNet.Modular package APIs.
  • ✅ The root Directory.Build.props/Directory.Build.targets split changes when NullableAttributes.cs is compiled (now only for netstandard2.0/.NETFramework instead of unconditionally for every TFM), but the file's own #if NETSTANDARD2_0 || NETCOREAPP2_0 ... || NET48 guards already made it a no-op on net8.0/net9.0/net10.0, so this is a build-time no-op cleanup, not a behavior change.

✅ New quantities or units

  • ✅ No new quantities or units are introduced; not applicable to this PR.

✅ Changes to generated code

  • ✅ No changes to code generators or generated quantity code (Length, Temperature, Level, etc. are untouched); not applicable to this PR.

⚠️ CI / build configuration

  • ⚠️ .github/workflows/unitsnet-modular-ci.yml's paths: trigger list (for both push and pull_request) already listed Directory.Build.props but was not updated to include the two new root files this PR adds, Directory.Build.targets and Directory.Solution.targets — both are now imported into the UnitsNet.Modular build (the latter drives the LocalPackages pre-pack step). A future edit to either file won't trigger this workflow.
  • ⚠️ The new "Run published-package consumer" CI step pulls UnitsNet.Modular from nuget.org at UnitsNetModularPublishedVersion (default 6.0.0-alpha.2, set in UnitsNet.Modular/Directory.Packages.props). Worth double-checking that version is actually published before this merges, since a missing package would break CI (unlike the LocalPackages/ProjectReferences steps, which are self-contained).
  • ✅ Isolating RestorePackagesPath per run/attempt (${{ runner.temp }}/unitsnet-modular-custom-packages-${{ github.run_id }}-${{ github.run_attempt }}) for the LocalPackages CI step is a good fix for potential stale-cache/version-collision issues between runs.

✅ Code quality and MSBuild correctness

  • ✅ The new Directory.Build.props/Directory.Build.targets chain (root → UnitsNet.ModularUnitsNet.Modular/Samples) correctly re-imports each parent explicitly, which is required once a nearer Directory.Build.targets is introduced (MSBuild auto-discovery stops at the nearest file and does not chain automatically).
  • ValidateUnitsNetModularSampleDependencyMode fails fast with a clear <Error> if Platform isn't one of the three supported values, which is a nice guardrail for IDE users who pick an unexpected platform.
  • ✅ Consolidating the various one-off ConsumerOwned/NuGet.Sample/Compatibility.* sample projects into named, purpose-specific folders (GettingStartedSample, QuantitySelectionSample, CustomQuantitySample, SharedUnitsLibrarySample, compatibility Fixtures/) meaningfully improves discoverability versus the prior long/ambiguous names.
  • ⚠️ Verified there are no leftover references to the old paths/names (ConsumerOwned, DefinitionPackages, UnitsNet.Modular.Lean.Sample, UnitsNet.Modular.Custom.Sample, ImportDirectoryBuildProps=false) anywhere in the tree — none found, so this is just confirming cleanliness rather than a concern.

✅ Test coverage

  • UnitsNet.Modular.Compatibility.Tests fixtures were cleanly split into Fixtures/Generated, Fixtures/Legacy, and Fixtures/Shared with the same aliased-reference approach preserved; the renamed test (GeneratedRegistry_DescribesAndOperatesOnTheSelectedCatalog) reflects the ConsumerOwned → shared-units-library rename without any loss of coverage.

✅ Security concerns

  • ✅ No security-relevant changes identified (build/sample infra only).

@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

⚠️ Review: Non-blocking findings

This PR is scoped entirely to the experimental UnitsNet.Modular sample portfolio and its build infrastructure (75 files). It does not touch Common/UnitDefinitions/, CodeGen/, or UnitsNet/GeneratedCode/, so the usual generated-code / new-quantity criteria don't apply here — review focuses on the MSBuild restructuring, sample reorganization, CI, and docs.

✅ Breaking changes

  • ✅ No changes to the published UnitsNet / UnitsNet.Modular public API surface. This is sample and build-tooling reorganization only; nothing here affects consumers of the NuGet packages.

✅ New quantities or units

  • ✅ N/A — no unit definitions were added or modified.

✅ Changes to generated code

  • ✅ N/A — CodeGen/GeneratedCode are untouched.

⚠️ Build system / MSBuild changes

  • ✅ The new Platforms axis (ProjectReferences/LocalPackages/PublishedPackages) is scoped correctly via nearest-Directory.Build.props resolution — it only applies under UnitsNet.Modular/** and does not leak into the main UnitsNet.slnx projects.
  • ✅ Moving the NullableAttributes.cs compile-include into the new root Directory.Build.targets, gated on netstandard2.0/.NETFramework, is a safe tightening — the file's own #if guards already made it a no-op elsewhere, and no repo project outside those TFMs is affected.
  • ⚠️ Directory.Solution.targets's PrepareUnitsNetModularLocalPackagesForSolution target (root, new) relies on running once via BeforeTargets="_GenerateRestoreGraph;Restore" before NuGet's static-graph restore fans out per-project. This is a reasonable way to avoid the parallel-pack race the comment describes, but solution-level target ordering relative to per-project restore graph generation is a somewhat fragile MSBuild guarantee (e.g. behavior can differ under dotnet restore --getProperty or other static-graph paths). Since the PR description confirms a clean LocalPackages build was validated, this is likely fine, but worth double-checking with a cold restore (no local feed / no prior stamp) to be sure only one dotnet pack process launches.
  • ⚠️ The "Run published-package consumer" CI step builds GettingStartedSample.csproj with -p:Platform=PublishedPackages, resolving UnitsNet.Modular 6.0.0-alpha.2 pinned in UnitsNet.Modular/Directory.Packages.props. Please confirm that version is actually published on nuget.org before merging — if it isn't yet, this new CI step will fail restore.
  • ✅ The compatibility-test fixture split (Fixtures/Generated, Fixtures/Legacy, Fixtures/Shared) with Compile Remove="Fixtures/**/*.cs" at the test project level and per-fixture-csproj Compile Include of the shared scenario file is a sound way to compile the same source against both aliased implementations without duplicate/missing compilation.
  • SharedUnitsLibrarySample.Units.csproj and the other five sample projects consistently follow the same platform-switching pattern; SharedUnitsLibrarySample.Domain/.App correctly stay plain ProjectReference consumers of .Units rather than switching modes themselves.

✅ Cross-file consistency

  • ✅ All renamed sample paths (ModularPlayground, GettingStartedSample, QuantitySelectionSample, CustomQuantitySample, Profiles/AllSiProfileSample, SharedUnitsLibrarySample) are consistently updated across .devcontainer/unitsnet-modular/devcontainer.json, .vscode/launch.json, UnitsNet.Modular.slnx, ARCHITECTURE.md, and sample READMEs — no stale references to deleted paths (ConsumerOwned, UnitsNet.Modular.NuGet.Sample, UnitsNet.Modular.Lean.Sample, UnitsNet.Modular.AllSi.Sample, UnitsNet.Modular.Representative.Sample, old DefinitionPackages/... location) found.
  • ✅ No orphaned Directory.Build.props/.targets left behind under deleted sample folders (e.g. ConsumerOwned/Directory.Build.props is properly removed).

✅ Documentation

  • ARCHITECTURE.md and the new Samples/README.md clearly describe the six-sample portfolio and the platform-based dependency switching; the new per-sample README files are a nice addition for discoverability.

✅ Test coverage

  • ✅ Existing compatibility tests were relocated/adapted (not weakened) to the new fixture layout; PR description reports the existing 116 tests still pass across all three platforms.

Overall this is a well-executed cleanup of a previously sprawling sample set, with the Platform-based dependency switching being a nice simplification. The two ⚠️ items (cold-restore race verification, and confirming the pinned 6.0.0-alpha.2 package is published) are worth a quick check before merge but aren't blocking from a code-review standpoint.

@angularsen
angularsen merged commit f2a455a into master Aug 2, 2026
9 checks passed
@angularsen
angularsen deleted the agl-codex/improve-modular-samples branch August 2, 2026 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant