🔧ci: Cross-platform GitHub Actions CI

[angularsen]

Summary

• Migrate v6 CI and pull-request validation to Linux runners in GitHub Actions, using .NET 8, 9, and 10.
• Build every shipped target (netstandard2.0, net8.0, net9.0, and net10.0), and run the main test suite with coverage on net10.0.
• Keep net48 as an opt-in test target and run the complete suite with dotCover on Windows/CLR4 after every push to master (and on demand), uploading complementary netstandard2.0 coverage to Codecov under the net48 flag.
• Test only behaviorally distinct runtime paths: the latest .NET runtime in the main pipeline and the netstandard2.0 assets on Windows/CLR4 in the compatibility pipeline; bound all new jobs with explicit timeouts.
• Authenticate Codecov with short-lived GitHub OIDC credentials instead of a stored token.
• Keep the benchmark project's net48 target on Windows without blocking Linux solution builds.
• Publish v6 packages only from GitHub Actions to NuGet.org.
• Disable Azure CI and PR triggers for v6/master while preserving the branch-specific Azure pipeline on maintenance/v5 for nanoFramework.
• Include the repository README in all companion NuGet packages.
• Update CI documentation, workflow links, and README badges.

Validation

• CI Build #38 passed on Linux in 8m56s:
  • build, test, coverage, pack, and artifact upload succeeded
  • UnitsNet: 42,702 passed and 20 skipped on each of net8.0, net9.0, and net10.0
  • NumberExtensions: 1,720 passed on each runtime
  • NumberExtensions CS14: 1,720 passed on each runtime
  • Json.NET: 115 passed on each runtime
  • Codecov OIDC accepted all four coverage reports; project and patch checks passed
  • the Node 20 and irrelevant native/Python coverage warnings are gone
• Final PR Build passed on the final head with test-result publication, Codecov upload, artifact upload, and no missing-readme package warnings.
• ./Build/build.ps1 passed locally end-to-end after the final test-target refinement: every shipped target compiled, all four suites passed on net10.0 (46,257 tests passed and 20 skipped), and four coverage reports were generated.
• The exact Windows/CLR4 test commands from the new compatibility workflow passed locally:
  • UnitsNet: 42,691 passed and 20 skipped
  • NumberExtensions: 1,720 passed
  • NumberExtensions CS14: 1,720 passed
  • Json.NET: 115 passed
• A local dotCover/CLR4 probe passed all 115 Json.NET tests and produced a 6.3 MB detailed coverage report containing the instrumented UnitsNet netstandard2.0 assembly.
• The new net48 workflow cannot be dispatched on GitHub before merge because GitHub only exposes manually dispatchable workflows that already exist on the default branch. Its first push-to-master run will verify the hosted Windows integration.
• The changed GitHub Actions workflows pass actionlint; git diff --check passes.

Migration notes

• Remove Azure DevOps UI trigger overrides, schedules, branch policies, and status checks targeting master after merging.
• Keep the Azure pipeline definition and policies required by maintenance/v5.

[claude]

Review: CI/build modernization + netstandard2.0/net48 test coverage

This PR is primarily CI/build infrastructure (no new quantities/units, no changes to generated quantity code). Summary below.

Breaking changes

None for library consumers. All changes are internal test/CI/build tooling. The IQuantity.QuantityInfo / IQuantityOfType<T> shapes are untouched — only test scaffolding (HowMuch.cs) was updated to match the pre-existing conditional interface members under !NET.

CI/workflow changes

• Good cleanup: adding permissions: contents: read and concurrency groups to ci.yml/pr.yml/net48-compatibility.yml are solid security/hygiene wins (least-privilege GITHUB_TOKEN, cancel stale runs).
• Switching pr.yml/ci.yml build-and-test to ubuntu-latest while keeping a separate scheduled net48-compatibility.yml on windows-latest for the CLR4 matrix is a sensible split.
• net48-compatibility.yml runs on a schedule + workflow_dispatch only — worth confirming that's intentional (regressions on net48 won't block PRs/CI, only surface weekly or on manual trigger). Given net48's slow pace of change, that tradeoff seems reasonable, but flagging it explicitly.
• publish-nuget job: the new Azure Artifacts steps are gated with if: env.AZURE_DEVOPS_PAT != '', but the Configure Azure Artifacts step sets source-url and expects NUGET_AUTH_TOKEN env — worth double-checking that actions/setup-dotnet@v6's source-url input actually authenticates NuGet.config with NUGET_AUTH_TOKEN as expected (standard pattern, likely fine, just unexercised until a PAT secret exists).
• Nice fix: replacing the hand-rolled GPG-verified Codecov download/upload script with codecov/codecov-action@v5 removes a lot of fragile, Windows-specific shell logic.

Code quality / generated code

The only generator change is in CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs:

- var units = Enum.GetValues<{_unitEnumName}>();
+ var units = EnumHelper.GetValues<{_unitEnumName}>();

This is needed because Enum.GetValues<T>() (the generic overload) doesn't exist before .NET 5/net48, and this test now needs to compile under the new net48/netstandard2.0 test targets. The change is applied consistently and mechanically across all ~130 generated *TestsBase.g.cs files — confirmed for a representative sample across quantity kinds: LengthTestsBase.g.cs (ILinearQuantity), TemperatureTestsBase.g.cs (IAffineQuantity), and LevelTestsBase.g.cs (ILogarithmicQuantity) all get the identical one-line substitution. EnumHelper (UnitsNet/InternalHelpers/EnumHelper.cs) is a small internal wrapper that dispatches to Enum.GetValues<TEnum>() on NET7_0_OR_GREATER and falls back to Enum.GetValues(typeof(TEnum)).Cast<TEnum>().ToArray() otherwise — straightforward and correct, and UnitsNet.Tests already has InternalsVisibleTo access.

The same substitution is applied by hand in UnitsNet.Tests/CustomCode/PressureTests.cs (Enum.GetValues<PressureReference>() → EnumHelper.GetValues<PressureReference>()).

Potential bug / correctness (worth calling out explicitly)

UnitsNet/Extensions/LogarithmicQuantityExtensions.cs: the !NET fallback for RootN used to be Math.Pow(number, 1.0 / n), which silently returns NaN for negative number since 1.0/n is essentially never an integer exponent. The new fallback special-cases the sign (via BitConverter.DoubleToInt64Bits(number) < 0) and mirrors double.RootN's semantics (real negative root for odd n, NaN for even n). This is a genuine behavior fix, not just a refactor — worth calling out explicitly in the PR description since it changes runtime results on netstandard2.0/net48 for negative inputs to GeometricMean and similar logarithmic-quantity helpers.

Test coverage

• Good additions: NetStandard20AssetTests.cs (added to UnitsNet.Tests, UnitsNet.NumberExtensions.Tests, UnitsNet.NumberExtensions.CS14.Tests, UnitsNet.Serialization.JsonNet.Tests) assert the referenced assembly's TargetFrameworkAttribute is actually .NETStandard,Version=v2.0 when TestNetStandard20=true — a nice guard against silently testing against the wrong build output.
• Minor nit: UnitsNet.Tests/LogarithmicQuantityExtensionsTest.cs now has its own private RootN helper (identical to the new prod fallback) instead of calling double.RootN directly, because the test project can now also target net48 where double.RootN doesn't exist. Understandable given the net48 target, but it means the test's expected values are computed with the same formula as production rather than an independent (BCL) reference — if there were ever a subtle bug in the sign-handling, both would agree and the test wouldn't catch it. Not blocking, just worth knowing.
• The UnitsNetBaseJsonConverterTest/UnitsNetIComparableJsonConverterTest/UnitsNetIQuantityJsonConverterTest changes from asserting the full localized ArgumentNullException.Message string to asserting .ParamName instead are a solid robustness improvement — ArgumentNullException message text has changed across .NET versions/runtimes, so this was likely already a source of cross-TFM flakiness.

Other

• HowMuch.cs (custom test quantity): the #if !NET guard became #if !NET || TEST_NETSTANDARD20, and the explicit interface member switched from QuantityInfo IQuantity.QuantityInfo => Info; to IQuantityInstanceInfo<HowMuch> IQuantityOfType<HowMuch>.QuantityInfo => Info;. Traced through IQuantity.cs: under !NET, IQuantityOfType<TQuantity> redeclares QuantityInfo as IQuantityInstanceInfo<TQuantity> (not the base QuantityInfo type), so this now matches the actual interface shape produced when UnitsNet.csproj is built for netstandard2.0/net48. Since the test project forces NET to stay defined for its own conditional code while adding TEST_NETSTANDARD20 to select this branch, this correctly mirrors what a real netstandard2.0/net48 consumer would need to implement. Subtle — a short comment explaining why TEST_NETSTANDARD20 needs to be OR'd in here (despite NET being simultaneously defined in the test project) would help future readers.
• README.md/GITHUB_ACTIONS_MIGRATION_README.md updates clearly describe the maintenance/v5 (Azure DevOps, nanoFramework) vs master (GitHub Actions v6) split.

Overall: a large but low-risk, well-scoped infra PR. The one substantive behavior change (RootN negative-number fallback) should be called out explicitly in the PR description/changelog since it's a real bugfix bundled into what's otherwise framed as a CI migration.

---

Reviewed by Claude

[claude]

Review: #1686 — Cross-platform GitHub Actions CI

This PR migrates CI from Azure Pipelines to GitHub Actions (Linux runners), adds a scheduled .NET Framework 4.8 compatibility workflow, and adds netstandard2.0 asset test coverage. No unit/quantity definitions are touched. Overall solid and well-scoped; a few items below.

Breaking changes

None for library consumers — this is CI/build/test infrastructure only. The public API surface is unchanged. The HowMuch.cs conditional-compilation tweak (UnitsNet.Tests/CustomQuantities/HowMuch.cs) only adjusts which explicit interface members a test-only custom quantity implements under !NET || TEST_NETSTANDARD20, and both IQuantityOfType<T>/IQuantityInstanceInfo<T> already exist in UnitsNet/IQuantity.cs / IQuantityInfo.cs, so it's just adapting to an existing interface, not introducing one.

Generated code changes

CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs swaps Enum.GetValues<TUnit>() for EnumHelper.GetValues<TUnit>() in the HasAtLeastOneAbbreviationSpecified test. This is a correct, necessary fix: Enum.GetValues<T>() requires .NET 7+, and this PR is what first exercises the generated tests against netstandard2.0 and net48 targets. The change is applied identically and mechanically across all ~130 regenerated *TestsBase.g.cs files regardless of quantity kind — confirmed the same one-line diff in LengthTestsBase.g.cs (linear), TemperatureTestsBase.g.cs (affine), and LevelTestsBase.g.cs (logarithmic). Since EnumHelper already existed in UnitsNet/InternalHelpers/EnumHelper.cs, this is just closing a gap where the generator hadn't been updated to use it. Good, low-risk change.

Potential bug: Azure Artifacts push likely fails silently on missing auth

In .github/workflows/ci.yml, the publish-nuget job:

- name: Configure Azure Artifacts
  if: env.AZURE_DEVOPS_PAT != ''
  uses: actions/setup-dotnet@v6
  with:
    source-url: https://pkgs.dev.azure.com/...
  env:
    NUGET_AUTH_TOKEN: ${{ secrets.AZURE_DEVOPS_PAT }}

- name: Push to Azure Artifacts
  if: env.AZURE_DEVOPS_PAT != ''
  run: dotnet nuget push "**/*.nupkg" --skip-duplicate --api-key AzureArtifacts
  working-directory: nugets

actions/setup-dotnet's source-url writes a NuGet.Config entry that references %NUGET_AUTH_TOKEN% at usage time, not embedding the secret value at config-generation time — per the action's docs, NUGET_AUTH_TOKEN needs to be set in every subsequent step that touches that source. The "Push to Azure Artifacts" step has no env: block, so NUGET_AUTH_TOKEN is unset there and the push will likely fail with a 401. Since this only gates the optional Azure Artifacts mirror (guarded by AZURE_DEVOPS_PAT being present) and runs after the nuget.org push, it won't block the primary release, but it should be fixed by adding the same env: NUGET_AUTH_TOKEN: ${{ secrets.AZURE_DEVOPS_PAT }} to the push step.

Style/conventions

• Good practice: Push to nuget.org now passes the secret via env: NUGET_ORG_APIKEY and references it as "$NUGET_ORG_APIKEY" in the run: block instead of interpolating ${{ secrets.* }} directly into the shell command — avoids the secret ending up literally in the process/command line and is the recommended GitHub Actions pattern.
• permissions: contents: read (plus checks: write where needed) and concurrency groups with cancel-in-progress: true are sensible additions that weren't there before.
• The pr.yml "Publish test results" step now gates on github.event.pull_request.head.repo.full_name == github.repository, correctly skipping check-run publishing for fork PRs where the default GITHUB_TOKEN lacks write permission — good fix, avoids spurious CI failures for external contributors.
• Build/build-functions.psm1 and Build/init.ps1 switching from "$root\path" string concatenation to Join-Path is a nice cross-platform correctness improvement (needed now that the build also runs on Linux runners), and the OS-conditional reportgenerator/reportgenerator.exe binary name is handled correctly.

Bug fix bundled into this PR

UnitsNet/Extensions/LogarithmicQuantityExtensions.cs's netstandard2.0/net48 fallback for RootN previously did Math.Pow(number, 1.0 / n), which returns NaN for a negative base with fractional exponent (e.g. cube root of -8). The new implementation special-cases negatives correctly (odd root of negative → negate the root of the absolute value; even root of negative → NaN), matching double.RootN's actual .NET semantics. This is a legitimate, welcome bug fix, though it's somewhat orthogonal to a "CI migration" PR — worth calling out in the PR description since it changes runtime behavior for consumers still on net48/netstandard2.0, not just test/CI infra. The test file duplicates the same logic as a private helper rather than reusing the (rightly) private implementation — acceptable given visibility constraints, but worth a comment noting the duplication is intentional so it doesn't drift silently if the source implementation changes again.

Test coverage

The new NetStandard20AssetTests.cs (added identically to 4 test projects) is a nice sanity check that netstandard2.0 project references actually resolve to the netstandard2.0 asset rather than silently falling back to another TFM. The exception-assertion changes in the JsonNet serializer tests (Assert.Equal(paramName, exception.ParamName) instead of matching the full exception message) are a good robustness improvement — ArgumentNullException.Message text is runtime/culture-dependent and this decouples the tests from that.

Minor

• Build/build-functions.psm1's new netstandard2.0 test loop re-runs the full test suite for all 4 test projects a second time (--framework net8.0 -p:TestNetStandard20=true), roughly doubling local/CI build script test time. Intentional given the goal, but worth confirming CI duration is acceptable.
• Several third-party action versions are bumped to majors newer than what's publicly available as of my knowledge cutoff (actions/checkout@v7, actions/setup-dotnet@v6, actions/upload-artifact@v7, actions/download-artifact@v8). I can't verify these exist — worth double-checking they resolve, since a bad tag would fail the workflow outright.

No security concerns at medium+ severity, and no new quantities/units in this PR so .agents/criteria-for-adding-quantities-and-units.md doesn't apply.

[claude]

Review

Reviewed the CI/build migration to GitHub Actions plus the incidental code changes it carries along. Overall this is a well-scoped, carefully executed infra PR — no new quantities/units, and the generated-code diff is mechanical and consistent everywhere.

Breaking changes

None for library consumers. All changes are to CI, build scripts, test infrastructure, and test-only code. The one production source change (UnitsNet/Extensions/LogarithmicQuantityExtensions.cs) only affects the #if !NET fallback path (netstandard2.0/net48 builds), and is a behavior fix, not a break (see below).

Possible doc/workflow mismatch

The PR description says the net48 workflow will "run the complete suite weekly (and on demand) on Windows/CLR4", but .github/workflows/net48-compatibility.yml only defines:

on:
  push:
    branches:
      - master
  workflow_dispatch:

There's no schedule:/cron trigger. The updated README.md text ("...runs the complete test suite on CLR4 after every push to master...") matches the actual workflow, but not the PR description. Worth confirming whether a weekly cron was intended and simply dropped, or whether the PR description is just stale.

Changes to generated code

CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs swaps one remaining Enum.GetValues<{_unitEnumName}>() call for the existing EnumHelper.GetValues<{_unitEnumName}>() helper (already used elsewhere in the same generator, e.g. line 327/355) — this is what fans out into the ~130 one-line diffs across UnitsNet.Tests/GeneratedCode/TestsBase/*.g.cs (verified in LengthTestsBase.g.cs, TemperatureTestsBase.g.cs, LevelTestsBase.g.cs — all identical mechanical change). Good, low-risk consistency cleanup; presumably needed since Enum.GetValues<T>() isn't available when the test host runs against a netstandard2.0-targeted UnitsNet.dll reference (the new TestNetStandard20 scenario), whereas EnumHelper.GetValues<T>() already handles that.

Incidental bug fix bundled in a CI PR

LogarithmicQuantityExtensions.RootN's non-NET fallback changed from:

return Math.Pow(number, 1.0 / n);

to a version that special-cases negative number with odd n (previously Math.Pow(-8, 1.0/3) returns NaN instead of -2). This is a legitimate bug fix, but it's scope creep for a PR framed as CI migration — worth calling out explicitly in the PR description/changelog since it changes runtime behavior on net48/netstandard2.0 targets. Also note UnitsNet.Tests/LogarithmicQuantityExtensionsTest.cs now duplicates this exact algorithm as a private RootN test helper (replacing calls to double.RootN) so expected values match the library's manual-fallback math bit-for-bit rather than the BCL's implementation. That's a reasonable reason to duplicate it, but it does mean the two copies must be kept in sync by hand if the algorithm changes again — maybe worth a comment linking them, or extracting to a shared internal helper both prod and test code call into.

Good, non-obvious fixes worth highlighting

• UnitsNet.Serialization.JsonNet.Tests/*.cs: replacing Assert.Equal("Value cannot be null. (Parameter 'x')", ex.Message) with Assert.Equal("x", ex.ParamName) is the right call — ArgumentNullException.Message text differs across runtimes/frameworks (and locales), so asserting on Message was already fragile and would likely have broken under the new net48/netstandard2.0 CI legs.
• pr.yml's "Publish test results" step now gates on github.event.pull_request.head.repo.full_name == github.repository before invoking EnricoMi/publish-unit-test-result-action. Good — avoids failing/needing elevated permissions for forked-repo PRs.
• ci.yml/pr.yml now set permissions: contents: read (plus checks: write on pr.yml) explicitly rather than relying on default token permissions — good least-privilege practice.

Style/consistency nit

UnitsNet.Tests/CustomQuantities/HowMuch.cs's explicit interface implementation was updated from #if !NET to #if !NET || TEST_NETSTANDARD20, changing the member from QuantityInfo IQuantity.QuantityInfo to IQuantityInstanceInfo<HowMuch> IQuantityOfType<HowMuch>.QuantityInfo => Info;. This looks correct (matches the interface actually needed on non-static-abstract-member targets) but it's a fairly quiet fix bundled with the CI plumbing — a one-line comment on why this shape changed would help future readers who aren't tracing through the IQuantityOfType<T> interface hierarchy.

Test coverage

Nice addition: NetStandard20AssetTests.cs (added to UnitsNet.Tests, UnitsNet.NumberExtensions(.CS14).Tests, UnitsNet.Serialization.JsonNet.Tests) asserts via TargetFrameworkAttribute that the referenced assembly really is the netstandard2.0 build when TEST_NETSTANDARD20 is set — a good guard against the SetTargetFramework project-reference trick silently no-op'ing and giving false confidence that the netstandard2.0 asset was exercised.

Action version pins

actions/checkout@v7, actions/setup-dotnet@v6, actions/upload-artifact@v7, actions/download-artifact@v8, codecov/codecov-action@v5 — please double check these major versions are actually published/stable at merge time; if any of these are wrong/nonexistent the workflow will fail outright on the next run.

Nothing here blocks merging — mostly a couple of things to double-check (the missing cron trigger, action version pins) and two spots where non-CI behavior changes ride along and would benefit from a one-line callout in the PR description or a code comment.

[claude]

Review: #1686 — Cross-platform GitHub Actions CI

This is a CI/infra migration PR (158 files, mostly workflows + mechanical generated-test changes). Here's my review against the usual checklist.

Breaking changes

None to the public API. One behavior-relevant fix worth calling out: UnitsNet/Extensions/LogarithmicQuantityExtensions.cs — the netstandard2.0/net48 fallback for RootN previously did Math.Pow(number, 1.0/n), which returns NaN for negative number even with an odd n (e.g. cube root of -8). The new fallback special-cases sign/even-n to match double.RootN semantics used on net. This is a genuine bug fix that changes output for netstandard2.0/net48 consumers going from wrong (NaN) to correct — good catch, but worth a mention in the changelog since it's a behavior change, not pure CI plumbing.

New quantities/units

None added — not applicable here.

Generated code

Only one generator line changed, in CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs:
```csharp

• var units = Enum.GetValues<{_unitEnumName}>();

• var units = EnumHelper.GetValues<{_unitEnumName}>();
  ```
  This ripples mechanically through ~130 *TestsBase.g.cs files (e.g. LengthTestsBase.g.cs, TemperatureTestsBase.g.cs, LevelTestsBase.g.cs all get the identical one-line change). Makes sense: the generic Enum.GetValues<T>() overload is only available under NET7_0_OR_GREATER, and EnumHelper.GetValues<T>() already has the non-generic fallback needed to compile the test suite for net48. Clean, consistent, no logic change for the runtimes that already supported the generic overload.

Code quality / style

• Build/build-functions.psm1, Build/init.ps1, Build/build.ps1: good cross-plat cleanup — Join-Path instead of hardcoded \ separators, and an OS-aware reportgenerator/reportgenerator.exe resolution. Idiomatic PowerShell.
• .csproj conditionals consistently gate net48 behind '$(IncludeNet48)' == 'true' and $([MSBuild]::IsOSPlatform('Windows')), so a plain dotnet build on Linux (or without the flag) doesn't break — good guard.
• New NetStandard20AssetTests.cs in each test project (UnitsNet.Tests, NumberExtensions(.CS14), JsonNet) asserts the referenced assembly's TargetFrameworkAttribute is exactly .NETStandard,Version=v2.0 — a nice guard rail to make sure the "test the shipped netstandard2.0 asset" mechanism (SetTargetFramework=...netstandard2.0 + TestNetStandard20/TEST_NETSTANDARD20) is actually exercising what it claims to.

Potential bugs / things to double-check

• Several action versions jump noticeably (actions/checkout@v7, actions/upload-artifact@v7, actions/download-artifact@v8, actions/setup-dotnet@v6, codecov/codecov-action@v5). Worth a final sanity check that these tags actually exist/are stable at merge time — a typo'd/nonexistent tag would hard-fail every run.
• ci.yml/pr.yml add concurrency: { cancel-in-progress: true }. For pr.yml that's clearly fine; for ci.yml (push-triggered, includes the publish-nuget job) a rapid second push to master could in theory cancel an in-flight dotnet nuget push. Low risk in practice given push cadence to master, but worth knowing it's possible.
• Build/build-functions.psm1 now runs the full test matrix twice (once normal, once with -p:TestNetStandard20=true against net8.0 host) and merges both sets of *.coverage.xml into one HtmlSummary — coverage numbers will reflect two runs of the same tests against different assemblies. Probably intentional/harmless, but flagging in case a specific per-target coverage report was expected instead.

Security

Net positive:

• New permissions: contents: read (+ checks: write where needed) on all three workflows — good move to least-privilege, workflows previously ran with default (often broader) token permissions.
• NuGet publish step now passes the API key via env: NUGET_ORG_APIKEY and "$NUGET_ORG_APIKEY" in the script rather than interpolating ${{ secrets.NUGET_ORG_APIKEY }} directly into the run body — reduces risk of the secret leaking into logs or being affected by shell-injection via the interpolation.
• pr.yml's "Publish test results" step is now gated on github.event.pull_request.head.repo.full_name == github.repository, correctly skipping the step (and its checks: write usage) for PRs from forks.
• The old hand-rolled codecov download + GPG-verify shell script is replaced with the official codecov/codecov-action@v5 — removes a chunk of custom trust logic in favor of a maintained action, generally a good security simplification.

Test coverage

Good — the NetStandard20AssetTests additions are a solid, minimal-footprint way to assert the CI is actually exercising the shipped netstandard2.0 binaries rather than accidentally testing the higher-TFM build. The RootN fallback fix would benefit from an explicit negative-odd-root test case in LogarithmicQuantityExtensionsTest.cs (currently the test-local RootN helper mirrors the fix but I didn't see a case asserting the negative-input behavior itself).

Overall this looks like a well-executed, low-risk infra migration with one nice incidental bug fix. Nothing blocking.

[claude]

Review: 🔧ci: Cross-platform GitHub Actions CI

This is a pure CI/build-infra PR — no new quantities/units, no public API surface changes. Overall solid work; the netstandard2.0-asset testing is a genuine improvement that already found (and fixed) a real bug. A few things worth a look before merging.

Highlights

• UnitsNet/Extensions/LogarithmicQuantityExtensions.cs: the non-NET RootN fallback previously did Math.Pow(number, 1.0/n), which returns NaN for negative number with odd n (e.g. cube root of −8). The new fallback special-cases the sign and is correct. This bug was latent because CI never actually executed the shipped netstandard2.0/net48 assemblies before — nice catch, and a good justification for the new TestNetStandard20 test mode.
• Same root cause fixed in UnitsNet.Tests/CustomQuantities/HowMuch.cs — its #if !NET explicit interface implementation (QuantityInfo IQuantity.QuantityInfo) no longer matched the current IQuantityOfType<TQuantity> interface shape and would have failed to compile for netstandard2.0/net48. Also effectively "dead" until this PR's coverage made it live.
• CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs: Enum.GetValues<T>() → EnumHelper.GetValues<T>() in the generated HasAtLeastOneAbbreviationSpecified() test. Enum.GetValues<T>() is netstandard2.1+/.NET 5+ only, so this was blocking net48/netstandard2.0 compilation. Verified the regenerated .g.cs diffs are uniform across Length, Temperature, and Level test bases (and consistent with the two other pre-existing EnumHelper.GetValues<T>() call sites in the generator) — this is the single source-of-truth change producing all ~130 mechanical .g.cs file diffs.
• Switching Assert.Equal("Value cannot be null. (Parameter 'x')", ex.Message) to Assert.Equal("x", ex.ParamName) in the JsonNet tests is a good, low-risk fix — ArgumentNullException.Message wording differs across runtimes/frameworks and shouldn't be pinned in tests.
• Build/build-functions.psm1 / init.ps1: Join-Path instead of hardcoded \-based paths, and a conditional reportgenerator/reportgenerator.exe tool name — correct fix for cross-platform (Linux) execution.
• Azure Pipelines is cleanly neutered for master (trigger: none, pr: none, no-op job) while explicitly preserving maintenance/v5's pipeline — matches the stated migration intent.

Questions / possible issues

1. Pinned Action versions — actions/checkout@v7, actions/upload-artifact@v7, actions/download-artifact@v8, actions/setup-dotnet@v6 are newer major versions than I could verify from this environment (no outbound web access). Worth a final sanity check that these tags actually exist before merge, since a bad tag fails the whole workflow immediately.
2. UnitsNet.Tests/LogarithmicQuantityExtensionsTest.cs — the test now hand-duplicates the exact fallback algorithm from LogarithmicQuantityExtensions.RootN (sign-handling included) into a private RootN in the test file, instead of using double.RootN (available on all target frameworks here, since the test project itself always targets net8.0+ even when exercising the netstandard2.0 asset). This makes the assertion closer to "the same formula equals itself" than an independent correctness check. Not a blocker, but consider keeping double.RootN as the reference value, or at least a comment explaining why it was inlined.
3. net48-compatibility.yml publishes results via EnricoMi/publish-unit-test-result-action/windows@v2, a third-party action — same version-pin caveat as Added short and long tons. #1, and it's a supply-chain dependency worth being deliberate about (seems pre-existing though, not new to this PR for the Windows variant).
4. Minor: codecov/codecov-action@v5 in pr.yml has fail_ci_if_error: false while ci.yml has fail_ci_if_error: true — intentional (don't want flaky Codecov uploads to block/red PR checks, but do want it strict on master)? Worth confirming that's deliberate rather than a copy-paste inconsistency.

Test coverage

The new NetStandard20AssetTests.cs files (asserting TargetFrameworkAttribute.FrameworkName == ".NETStandard,Version=v2.0") are a nice guardrail to ensure the "exact shipped asset" test mode is actually wired correctly and not silently falling back to the net8.0 build.

Style/conventions

No violations of AGENTS.md conventions spotted — no unit/quantity JSON changes, generated code changes trace back to a single generator source change as expected, file headers preserved on new files.

No security concerns beyond the general secrets-handling note above (all secret usage looks conventional — env-var indirection into dotnet nuget push, no pull_request_target, no secrets exposed to fork PR runs).

🤖 Generated with Claude Code

[claude]

Review: #1686 — Cross-platform GitHub Actions CI

This is a CI/build-infrastructure PR with no changes to unit definitions or public API surface.

Breaking changes

None. No Common/UnitDefinitions/*.json changes, no public API changes. All source edits are to workflows, PowerShell build scripts, and test code (mostly test-only conditional compilation plus one internal-helper swap).

New quantities or units

None added.

Generated code changes

Only one generator template line changed, in CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs:

-            var units = Enum.GetValues<{_unitEnumName}>();
+            var units = EnumHelper.GetValues<{_unitEnumName}>();

This is mechanically applied to every *TestsBase.g.cs file's HasAtLeastOneAbbreviationSpecified() test — confirmed consistent across LengthTestsBase, TemperatureTestsBase, LevelTestsBase, and the rest of the ~130 generated test files, so codegen was clearly re-run correctly and matches the generator template.

EnumHelper.GetValues<T>() (UnitsNet/InternalHelpers/EnumHelper.cs) wraps Enum.GetValues<T>() on NET7_0_OR_GREATER and falls back to Enum.GetValues(typeof(T)).Cast<T>().ToArray() otherwise. Since the test host itself always runs on net8.0+ regardless of the TEST_NETSTANDARD20 flag, this swap doesn't change the test's actual behavior — it's purely for consistency with the internal helper used elsewhere in the library. Harmless, but worth a one-line mention in the PR description for readers wondering why this line churns across 130 files in an otherwise CI-only PR.

Code quality / best practices

• The netstandard2.0 asset-testing approach (loading the shipped netstandard2.0 binaries into a net8.0 test host via SetTargetFramework + TestNetStandard20 MSBuild property + DisableImplicitFrameworkDefines) is a clever way to get real coverage of the netstandard2.0 fallback code paths without needing a runtime that can host netstandard2.0 directly. The new NetStandard20AssetTests.TestsReferenceNetStandard20Asset tests, which assert on TargetFrameworkAttribute, are a good safety net against the project-reference override silently failing to apply.
• Build/build-functions.psm1 / init.ps1 cross-platform fixes (Join-Path instead of hardcoded \, OS-conditional reportgenerator executable name) look correct and necessary for Linux runners.
• Good secret-handling improvement in ci.yml: the NuGet push step now passes the API key via env: + shell variable interpolation ("$NUGET_ORG_APIKEY") instead of embedding ${{ secrets.NUGET_ORG_APIKEY }} directly in the run: script, reducing the risk of the secret leaking into logs.
• Added permissions: blocks (contents: read, checks: write where needed) and concurrency groups to all three workflows — solid hardening and avoids redundant concurrent runs.
• pr.yml's test-result publishing step now gates on github.event.pull_request.head.repo.full_name == github.repository, avoiding failures on forked PRs where the action lacks write access. Good defensive addition.
• Worth a second look: ci.yml's concurrency: { group: ci-..., cancel-in-progress: true } applies to the whole workflow, including the publish-nuget job. If two pushes land on master in quick succession, an in-flight NuGet publish could in theory be cancelled mid-push by the newer run. Likely an acceptable edge case, but worth confirming it's a conscious choice.

Test coverage

• Good addition: NetStandard20AssetTests across UnitsNet.NumberExtensions.Tests, UnitsNet.NumberExtensions.CS14.Tests, and UnitsNet.Serialization.JsonNet.Tests (mirroring the pre-existing one in UnitsNet.Tests), verifying the loaded assembly really is the netstandard2.0 build so a broken SetTargetFramework override fails loudly instead of silently testing the wrong assembly.
• The ArgumentNullException assertions were changed from matching the full CLR-generated message ("Value cannot be null. (Parameter 'x')") to just asserting ParamName. Good robustness fix — that message text differs between .NET Framework and modern .NET/netstandard2.0 runtimes, which would otherwise make these tests brittle across the new multi-TFM matrix.
• IQuantityTests.StaticAbstract_Info_ReturnsSameAsTypedInfo is correctly excluded (#if NET && !TEST_NETSTANDARD20) when running against the netstandard2.0 asset, since static abstract interface members aren't available there.

Potential issues / nits

• GITHUB_ACTIONS_MIGRATION_README.md reads as ongoing CI documentation now rather than a one-time migration note, but its own "Migration notes" section implies it (or parts of it) should be cleaned up/removed post-merge. Consider moving durable parts into Docs/ so this doesn't go stale as a forgotten root-level file.

Overall this is a well-scoped, carefully validated infra migration — the author reports both Linux netstandard2.0 and Windows net48 suites passing, plus actionlint and git diff --check. No blocking concerns from this review.

[claude]

Review: #1686 — Cross-platform GitHub Actions CI

This is an infra-only PR (no new quantities/units, no changes to the shipped public API), so I focused on the CI/build changes, the generated-code template change, and a couple of incidental fixes bundled in.

Breaking changes

None for consumers of the NuGet packages. All production changes are either CI/build tooling or #if !NET polyfill internals (LogarithmicQuantityExtensions.RootN) whose observable behavior only improves (see below).

Changes to generated code

CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs swaps the HasAtLeastOneAbbreviationSpecified test template from Enum.GetValues<T>() to EnumHelper.GetValues<T>(), which cascades into all ~130 *TestsBase.g.cs files (e.g. LengthTestsBase.g.cs, TemperatureTestsBase.g.cs, LevelTestsBase.g.cs all get the identical one-line swap). This is necessary because Enum.GetValues<TEnum>() is only available under NET7_0_OR_GREATER, and this PR makes net48 a real (opt-in) test target again — EnumHelper.GetValues<T>() (UnitsNet/InternalHelpers/EnumHelper.cs) already had a net48-safe fallback and is InternalsVisibleTo-exposed to the test assembly, so this is the correct, minimal fix rather than a hand-rolled duplicate. Good use of existing infra.

Code quality / good practices

• concurrency: groups added to both ci.yml and pr.yml to cancel superseded runs, and explicit permissions: blocks scoped to least privilege (contents: read, plus id-token: write/checks: write only where needed) — nice hardening.
• Codecov upload now goes through codecov/codecov-action@v6 with use_oidc: true instead of the old hand-rolled script that downloaded the codecov binary, imported a GPG key, and verified a SHA256SUM by string comparison. This removes a chunk of security-sensitive shell logic and a long-lived CODECOV_TOKEN dependency for the public-repo path — good simplification and reduces attack surface.
• if-no-files-found: error added to the nuget-packages artifact upload in ci.yml — catches a silently-empty pack step.
• The publish-unit-test-result-action step is now gated on github.event.pull_request.head.repo.full_name == github.repository, which avoids the well-known failure mode of that action lacking permissions on fork PRs. Good defensive addition.
• Build/build-functions.psm1 / init.ps1 / build.ps1 moving from "$root\..." string concatenation to Join-Path is the right way to make the PowerShell scripts cross-platform.

Minor suggestions (non-blocking)

• Third-party actions (EnricoMi/publish-unit-test-result-action@v2, codecov/codecov-action@v6) are pinned by floating major-version tag rather than commit SHA. Since these run with checks: write / id-token: write, consider pinning by SHA for supply-chain hardening — optional given this is a well-known, actively maintained action, but worth considering given the CI now also has id-token: write for OIDC.
• UnitsNet.Tests/LogarithmicQuantityExtensionsTest.cs adds a private RootN helper that's a verbatim copy of the !NET fallback in UnitsNet/Extensions/LogarithmicQuantityExtensions.cs (needed because double.RootN doesn't exist on net48, and the production method is private). Understandable given InternalsVisibleTo doesn't help with private members, but flagging the duplication in case a future change to the polyfill's edge-case handling needs to stay in sync in two places.
• UnitsNet.Tests/CustomQuantities/HowMuch.cs changes the explicit interface implementation from IQuantity.QuantityInfo to IQuantityInstanceInfo<HowMuch> IQuantityOfType<HowMuch>.QuantityInfo — this matches the shape already used by all generated quantities (e.g. Density.g.cs), so it looks like a necessary straggler fix to keep the hand-written custom-quantity test fixture compiling, rather than something CI-related. Worth a one-line mention in the PR description since it's a substantive (if small) code change riding along in an otherwise infra-only PR.

Bug fix bundled in (worth calling out)

LogarithmicQuantityExtensions.cs's !NET RootN fallback previously did Math.Pow(number, 1.0 / n), which returns NaN for a negative number with odd n (e.g. cube root of -8). The new implementation handles the sign explicitly and matches double.RootN's behavior on modern .NET. This is a genuine correctness fix for the netstandard2.0/net48 code path, not just a CI change — good catch, and nice that the new net48-compatibility.yml workflow will now actually exercise this path again.

Test coverage

Not reduced. Moving the full net48 suite to run post-merge (net48-compatibility.yml, triggered on push to master, not on PRs) trades slightly slower feedback on net48 regressions for much faster PR CI — a reasonable and clearly-documented trade-off (comment in the workflow file explains it and offers the escape hatch of adding a pull_request trigger if regressions start slipping through).

Security

No concerns at or above medium severity. The removal of the manual GPG/SHA256 verification script in favor of the official Codecov action + OIDC is a net improvement, not a regression. permissions: blocks look correctly minimized per job.

Overall: a well-structured, security-conscious CI migration. Nothing blocking; the suggestions above are all optional polish.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96%. Comparing base (022e1f3) to head (728a56c).

Additional details and impacted files

@@          Coverage Diff           @@
##           master   #1686   +/-   ##
======================================
  Coverage      96%     96%           
======================================
  Files         450     450           
  Lines       29342   29342           
======================================
  Hits        28310   28310           
  Misses       1032    1032           

☔ 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]

Review: CI migration to GitHub Actions (#1686)

This is a CI/build infrastructure PR — no new quantities/units, and only one line of generated-code-generator change, so I'll focus feedback there.

Breaking changes

None for library consumers. This only touches build/CI infra, test projects, and one internal test-generator template. No changes to shipped UnitsNet/GeneratedCode or public APIs.

Generated code / code generator changes

CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs swaps Enum.GetValues<{_unitEnumName}>() → EnumHelper.GetValues<{_unitEnumName}>() in the generated HasAtLeastOneAbbreviationSpecified test, propagating to all ~130 *TestsBase.g.cs files plus the hand-written PressureTests.cs. This is correct: Enum.GetValues<TEnum>() isn't available pre-.NET 5, and UnitsNet/InternalHelpers/EnumHelper.cs already has the right shim (Enum.GetValues(typeof(TEnum)).Cast<TEnum>() for older TFMs). I checked for other Enum.GetValues< call sites and this is the only one — no missed spots. Good, mechanical, low-risk change tied directly to the goal of actually running the net48/netstandard2.0 test suite in CI.

Other correctness fixes bundled in

A couple of real fixes travel with the CI migration commit:

• UnitsNet/Extensions/LogarithmicQuantityExtensions.cs: the #else (non-NET) fallback for RootN previously did Math.Pow(number, 1.0/n), which returns NaN for negative bases with odd n (e.g. cube root of −8) instead of matching double.RootN's real-valued result. Now handles sign explicitly. Good catch — but note UnitsNet.Tests/LogarithmicQuantityExtensionsTest.cs duplicates this exact fixed logic as a private RootN test helper (since it can't call the library's private method and double.RootN isn't available pre-.NET 7 either). Minor duplication; not worth blocking on, but a comment noting it mirrors the extension's fallback would help future readers avoid divergence.
• UnitsNet.Tests/CustomQuantities/HowMuch.cs: the explicit !NET interface implementation was fixed from QuantityInfo IQuantity.QuantityInfo to IQuantityInstanceInfo<HowMuch> IQuantityOfType<HowMuch>.QuantityInfo, matching the actual IQuantityOfType<TQuantity> interface shape in IQuantity.cs. Looks like this was silently wrong before and only surfaces once net48 compiles this file for real.
• Two ArgumentNullException message assertions changed from matching the full localized .Message to just .ParamName — more robust across runtimes/cultures, good change.

CI/build workflow changes

• Moving ci.yml/pr.yml build+test to ubuntu-latest, keeping only net48 on a separate windows-latest post-merge workflow (net48-compatibility.yml, push-to-master only, not on PRs) is a sensible split — it keeps PR feedback fast while still validating the netstandard2.0 assets on real CLR4 after merge.
• Replacing the hand-rolled Codecov binary download/GPG-verify script with codecov/codecov-action@v6 + OIDC (use_oidc: true) is a nice security/maintenance win (removes a chunk of ad-hoc supply-chain-sensitive script, no more static CODECOV_TOKEN needed for the public repo case).
• Added concurrency groups (cancel-in-progress) and least-privilege permissions: blocks to both workflows — good practice.
• pr.yml's "Publish test results" step is correctly gated on github.event.pull_request.head.repo.full_name == github.repository, avoiding failures for forked-repo PRs that can't get a checks: write token.
• Azure Pipelines files are reduced to inert trigger: none / pr: none stubs with a notice — reasonable interim approach; the PR description already calls out the manual follow-up needed (removing Azure DevOps UI-level triggers/branch policies) since those can't be expressed in YAML alone.
• Build/build-functions.psm1 / init.ps1 switch to Join-Path and resolve reportgenerator vs reportgenerator.exe by platform — correctly makes the build scripts run on Linux pwsh, matching the new ubuntu-latest runners.
• csproj TargetFrameworks conditionally add net48 only when IncludeNet48=true and on Windows — keeps Linux builds from trying (and failing) to target net48, while the Windows workflow explicitly opts in via -p:IncludeNet48=true. UnitsNet.Benchmark.csproj gets the same Windows-only treatureatment for its net48 target.

Test coverage

UnitsNet.Tests/CustomCode/IQuantityTests.cs wraps StaticAbstract_Info_ReturnsSameAsTypedInfo in #if NET, since it exercises static-abstract-interface-member access that isn't available under net48 — correctly scoped rather than deleted.

Nits

• UnitsNet.Benchmark.csproj/test csproj comment: "treatureatment" typo above is mine, ignore — no actual typos spotted in the diff itself.
• Consider whether the duplicated RootN test helper (mentioned above) should instead expose the library's RootN via [InternalsVisibleTo] to avoid drift between the real implementation and the test's copy — optional, low priority.

Overall this is a well-scoped, well-tested infra migration with good validation notes in the PR description (test counts on both Linux and Windows net48). No concerns blocking merge from my side.

[claude]

Correction to my review above: the "Nits" section had a stray typo ("treatureatment") — meant to say the Benchmark csproj's Windows-only net48 conditional is consistent with the other test csproj changes. No other typos found in the diff; review substance stands.

[claude]

Review: 🔧ci: Cross-platform GitHub Actions CI

This is primarily a CI/build infrastructure PR (153 files, but ~128 are a single mechanical line change repeated across generated test files). Overall it's a well-scoped, well-documented migration with good validation evidence in the PR description.

Breaking changes

None to the shipped library's public API. UnitsNet/UnitsNet.csproj and other library projects are untouched; the only production code change is in UnitsNet/Extensions/LogarithmicQuantityExtensions.cs (see below), which is a private helper, not public surface.

Not a new quantity/unit PR

No changes under Common/UnitDefinitions/, so the criteria in .agents/criteria-for-adding-quantities-and-units.md don't apply here.

Changes to generated code

• CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs changes one line: Enum.GetValues<{_unitEnumName}>() → EnumHelper.GetValues<{_unitEnumName}>() in the HasAtLeastOneAbbreviationSpecified test. This propagates correctly and consistently to all 128 generated *TestsBase.g.cs files (verified — no stragglers left on the old pattern). EnumHelper.GetValues<T> (pre-existing in UnitsNet/InternalHelpers/EnumHelper.cs) falls back to Enum.GetValues(typeof(T)).Cast<T>() on pre-.NET7 targets, since the generic Enum.GetValues<T>() overload isn't available on netstandard2.0/net48. This is a necessary and correctly mechanical fix to make the test suite compile against the net48 compatibility target. Spot-checked LengthTestsBase.g.cs (ILinearQuantity) and confirmed the using UnitsNet.InternalHelpers; is already present, so it compiles cleanly. No other generator logic changed — Temperature/IAffineQuantity and Level/ILogarithmicQuantity test bases get the same single-line change, nothing quantity-type-specific.

Bug fixes surfaced by the net48 target

• UnitsNet/Extensions/LogarithmicQuantityExtensions.cs: the !NET fallback for RootN previously did Math.Pow(number, 1.0 / n), which returns NaN for negative number with odd n (e.g. cube root of -8), diverging from double.RootN on modern .NET. The new implementation correctly handles the negative/odd-root case by operating on Math.Abs(number) and re-applying the sign. Good catch — this was presumably never exercised because net48 wasn't part of the automated test matrix before. Nice byproduct of doing the net48 migration properly rather than papering over it.
  • Minor coverage note: UnitsNet.Tests/LogarithmicQuantityExtensionsTest.cs duplicates this same RootN algorithm as a private test helper (since production RootN is private) to compute expected values, but no test exercises negative-input geometric means, so this exact fix isn't independently verified by a test — it just avoids a latent runtime bug. Consider a direct [Theory] test for RootN/GeometricMean with a negative value and odd root count to lock in the fix.
• UnitsNet.Tests/CustomQuantities/HowMuch.cs: the !NET explicit interface implementation was fixed from QuantityInfo IQuantity.QuantityInfo to IQuantityInstanceInfo<HowMuch> IQuantityOfType<HowMuch>.QuantityInfo => Info;. This looks like a real fixture bug — it didn't match the actual IQuantityOfType<TQuantity> interface member (IQuantityInstanceInfo<TQuantity> QuantityInfo) already on master, so it presumably never compiled under a real !NET (net48) build until now. Good that enabling net48 testing caught this.

CI/workflow review

• Sensible move: Linux runners for the main build/test matrix (net8/9/10), Windows kept only for the after-merge net48 compatibility check — this should meaningfully cut PR CI latency.
• permissions: contents: read at workflow level with narrow per-job elevation (id-token: write only on the job that needs Codecov OIDC) is good least-privilege practice, and dropping the stored CODECOV_TOKEN in favor of OIDC removes a persistent secret.
• net48-compatibility.yml triggers on push: branches: [master] only (not PRs), with a documented comment as to why (no net48 library target, only exercises netstandard2.0 assets). Reasonable trade-off, though it does mean net48 regressions land on master before being caught — as already called out in the PR description as a known limitation.
• Good fork-safety fix in pr.yml: the "Publish test results" step now gates on github.event.pull_request.head.repo.full_name == github.repository, avoiding failures on forked PRs where the default GITHUB_TOKEN can't write check runs. Also switched from the /windows variant of EnricoMi/publish-unit-test-result-action to the standard one, correctly following the runner move to ubuntu-latest.
• Minor inconsistency: actions/upload-artifact@v7 is used to publish nuget-packages in ci.yml, but publish-nuget downloads via actions/download-artifact@v8. These major versions are cross-compatible (both v4+ storage backend), but pinning matching majors would be cleaner and less confusing to future maintainers.
• Several third-party/official action versions bumped fairly aggressively (checkout@v7, setup-dotnet@v6, upload-artifact@v7/download-artifact@v8, codecov/codecov-action@v6). Worth double-checking these tags actually exist and are the intended ones before merge — a typo'd or non-existent version pin fails loudly, but it's cheap to verify given how many were bumped at once.
• Build/build-functions.psm1 and init.ps1: switching from "$root\..." string concatenation to Join-Path is the right way to make the PowerShell build script OS-path-agnostic for Linux runners, and the reportgenerator/reportgenerator.exe OS-conditional binary name is a needed companion fix. Straightforward and correct.

Test coverage

• UnitsNet.Serialization.JsonNet.Tests: switching assertions from matching the full exception Message string to just ParamName for ArgumentNullException cases is a good robustness improvement — full BCL exception message text is version/runtime-dependent (e.g., wording differs across .NET 8/9/10), so asserting on ParamName avoids spurious failures purely from expanding the test matrix, while still verifying the meaningful part of the exception.
• UnitsNet.Tests/CustomCode/IQuantityTests.cs: wrapping StaticAbstract_Info_ReturnsSameAsTypedInfo in #if NET is correct, since static abstract interface members aren't available pre-.NET7/net48.

Nits

• GITHUB_ACTIONS_MIGRATION_README.md rewrite reads well and is much more useful than the original "files to move" placeholder — consider whether this file should eventually live under Docs/ per the repo's existing documentation convention, or be folded into Docs/README.md's CI section, rather than staying at repo root long-term (not a blocker for this PR).

Overall: solid, low-risk migration with good self-review evidence in the PR description (actual CI run links, local validation, actionlint/git diff --check passing). The two incidental bug fixes (RootN sign handling, HowMuch fixture) are a good sign that the net48 target is doing its job of catching real gaps rather than just being a checkbox.

[claude]

Review: Cross-platform GitHub Actions CI

This is a CI/tooling migration PR — no new quantities/units, and no changes to the production quantity/unit generators. 156 files changed, but ~130 of those are a single mechanical line in generated test files, so the actual reviewable surface is small. Nice work overall; a few notes below.

Breaking changes

None to the public API. One behavior-relevant fix: LogarithmicQuantityExtensions.RootN's non-NET fallback (UnitsNet/Extensions/LogarithmicQuantityExtensions.cs) previously did Math.Pow(number, 1.0/n), which returns NaN for a negative base with odd n (e.g. RootN(-8, 3) should be -2 but returned NaN on netstandard2.0/net48). The new sign-extraction logic matches double.RootN's semantics on NET. This is a genuine bug fix and worth a line in the changelog since it changes output for negative inputs on non-NET targets, even though it's not something this PR set out to do.

Generated code changes

The only generator touched is CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs, which swaps Enum.GetValues<{_unitEnumName}>() for EnumHelper.GetValues<{_unitEnumName}>() in the HasAtLeastOneAbbreviationSpecified test. That's because the generic Enum.GetValues<T>() overload only exists on .NET 7+, and this PR reinstates net48/netstandard2.0 test coverage (via the new net48-compatibility.yml workflow), so the generated tests need the polyfill in UnitsNet/InternalHelpers/EnumHelper.cs. This propagates identically to every *TestsBase.g.cs file (confirmed on LengthTestsBase.g.cs, LevelTestsBase.g.cs, and others) — no quantity-specific generator logic (linear/affine/logarithmic) was touched, so Length, Temperature, and Level code generation is otherwise unaffected.

Code quality

• Good: UnitsNet.Serialization.JsonNet.Tests assertions changed from Assert.Equal("Value cannot be null. (Parameter 'x')", ex.Message) to Assert.Equal("x", ex.ParamName). Exact ArgumentNullException.Message wording is runtime/culture-dependent, so this is a more robust and more portable assertion — especially relevant now that tests run against net8/9/10 and net48 in the same suite.
• UnitsNet.Tests/LogarithmicQuantityExtensionsTest.cs reimplements the same RootN fallback logic as a private test helper (duplicating LogarithmicQuantityExtensions.RootN's non-NET branch) instead of reusing double.RootN conditionally like the production code does. Minor DRY concern — if the production fallback's math ever regresses, the test's copy won't catch it since it embeds the same logic. Not blocking, just worth a #if NET ... double.RootN ... #else mirror instead of a full reimplementation if convenient.
• UnitsNet.Benchmark.csproj's net48 target is gated only by IsOSPlatform('Windows'), whereas all the test .csproj changes gate net48 behind both IncludeNet48 == 'true' and IsOSPlatform('Windows'). That means a plain local Windows build (dotnet build) will still try to build/restore the net48 benchmark target even though net48 is otherwise opt-in everywhere else now. Probably fine since Benchmark isn't part of build.ps1's test loop, but the asymmetry looks unintentional — worth double-checking it doesn't surprise Windows contributors doing a full solution build.

CI/workflow changes

• Replacing the bespoke codecov.exe download + GPG signature verification script with codecov/codecov-action@v6 + use_oidc: true is a nice simplification and removes the stored CODECOV_TOKEN secret entirely.
• Security nit (fixed by this PR, worth calling out): the old ci.yml nuget-push step interpolated the secret directly into the shell command (--api-key ${{ secrets.NUGET_ORG_APIKEY }}), which is a known GitHub Actions injection footgun (expression expansion happens before the shell sees the string). The new version passes it via env: NUGET_ORG_APIKEY and references "$NUGET_ORG_APIKEY" inside the script — correct fix, good catch.
• Top-level permissions: contents: read with job-scoped id-token: write only where OIDC is needed is good least-privilege practice.
• pr.yml's new guard github.event.pull_request.head.repo.full_name == github.repository before publishing test results avoids a predictable failure on fork PRs (forked PR runs get a read-only GITHUB_TOKEN, which publish-unit-test-result-action needs write access for). Good defensive addition.
• net48-compatibility.yml declares permissions: contents: read / checks: write at the workflow level rather than per-job; harmless here since there's only one job, but inconsistent with ci.yml/pr.yml's per-job scoping — minor style nit only.
• Concurrency groups added to ci.yml/pr.yml (cancel superseded runs) are a good addition given the switch to a wider SDK matrix (8/9/10) per run.

Performance

• Moving build-and-test from windows-latest to ubuntu-latest for the main CI/PR workflows should meaningfully cut wall-clock time and Actions minutes cost, while net48-compatibility.yml preserves net48/CLR4 coverage on Windows without blocking PRs (push-to-master + manual dispatch only) — reasonable trade-off between coverage and PR feedback latency.
• -p:TestTfmsInParallel=false in Build/build-functions.psm1 serializes TFMs within a single dotCover session per the comment explaining .NET 9+ parallel test hosts hang under one profiler session. This trades some parallelism for reliability; given it's scoped only to the dotCover coverage run (not dotnet test in general), the cost should be limited to CI, not local dev iteration.

Test coverage

No new functional tests added (expected for a CI-focused PR), but the IQuantityTests.cs static-abstract-interface test is correctly wrapped in #if NET since static abstract interface members aren't available pre-.NET 7, and HowMuch.cs's non-NET custom IQuantityOfType<HowMuch>.QuantityInfo implementation was fixed to match the current interface shape — both necessary and correctly scoped to unblock reinstating net48 as a test target.

Summary

Solid, well-scoped infra PR. The two things I'd actually want addressed before merge are minor: the RootN behavior change deserves a changelog mention, and the UnitsNet.Benchmark.csproj net48 gating inconsistency is worth a quick check to confirm it's intentional. Everything else is either a clear improvement (Codecov OIDC, nuget secret handling, ParamName assertions) or a necessary mechanical consequence of reinstating net48 coverage.

[claude]

Review: 🔧ci: Cross-platform GitHub Actions CI

This is a CI/build-infrastructure PR (no Common/UnitDefinitions/ changes, no new quantities/units), so I focused on the workflow changes, the one generator change, and the incidental fixes it surfaced.

Breaking changes

None to the public API. There is one behavioral fix in UnitsNet/Extensions/LogarithmicQuantityExtensions.cs: the non-.NET fallback for RootN used Math.Pow(number, 1.0 / n), which returns NaN for a negative number with odd n (e.g. cube root of a negative dB sum in GeometricMean). The new code handles the sign explicitly. This only affects netstandard2.0/net48 consumers and is a correctness fix, not a regression — but worth calling out explicitly in the PR body/changelog since it changes runtime output for those targets.

Generated code

Only one generator template changed: CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs swaps Enum.GetValues<{_unitEnumName}>() for EnumHelper.GetValues<{_unitEnumName}>() in the HasAtLeastOneAbbreviationSpecified test. That's because the generic Enum.GetValues<TEnum>() isn't available on netstandard2.0/net48, which this PR newly exercises via the net48-compatibility.yml workflow. The change cascades mechanically into ~130 *TestsBase.g.cs files (e.g. LengthTestsBase.g.cs, TemperatureTestsBase.g.cs, LevelTestsBase.g.cs) with an identical one-line diff each — exactly the right way to do it (edit the generator, regenerate, don't hand-edit .g.cs). Good adherence to the repo convention.

Bugs surfaced by this change (good catches)

Enabling real test execution on net48 (previously untested) surfaced two latent bugs, both fixed correctly:

• UnitsNet.Tests/CustomQuantities/HowMuch.cs: the #if !NET branch implemented the old QuantityInfo IQuantity.QuantityInfo shape instead of the current IQuantityInstanceInfo<HowMuch> IQuantityOfType<HowMuch>.QuantityInfo, which would have failed to compile/behave correctly on net48.
• The RootN fallback bug mentioned above.

This is exactly the value of actually running the net48 suite rather than just compiling for it — nice validation that the new workflow works.

Minor nits

• UnitsNet.Tests/LogarithmicQuantityExtensionsTest.cs now has a private RootN that duplicates the production fallback algorithm verbatim (needed since double.RootN isn't available pre-.NET 7). Since the test literally re-implements the same logic being tested, it can't catch a mistake made identically in both places. Not blocking, just noting the test is closer to a golden-value/regression check than an independent verification.
• GITHUB_ACTIONS_MIGRATION_README.md at repo root reads like a temporary migration note — consider folding it into Docs/ or removing after merge per its own "Migration notes" section, rather than leaving a permanent root-level file.

Security

Good improvements here, nothing new to flag:

• Codecov auth moved from a stored CODECOV_TOKEN secret to short-lived OIDC (use_oidc: true + id-token: write scoped only to the build-and-test job).
• NUGET_ORG_APIKEY is now passed via env: + "$NUGET_ORG_APIKEY" in the run script instead of interpolated directly into the run: block — reduces script-injection risk if the secret value ever contained shell metacharacters.
• Explicit top-level permissions: contents: read with per-job escalation only where needed (id-token: write, checks: write).
• publish-unit-test-result-action step is now gated on github.event.pull_request.head.repo.full_name == github.repository, avoiding failures (and the GITHUB_TOKEN permission issues) on fork PRs.

Other observations

• Consistent net48 opt-in pattern across all four test .csproj files (TargetFrameworks Condition="'$(IncludeNet48)' == 'true' and $([MSBuild]::IsOSPlatform('Windows'))") — clean and matches the new net48-compatibility.yml workflow's -p:IncludeNet48=true.
• Sensible scope reduction: full multi-target build validates all shipped TFMs, but only net10.0 runs with coverage in the main pipeline, while net48-compatibility.yml covers the behaviorally-distinct netstandard2.0 assets on CLR4 post-merge without blocking PRs. Matches the PR description and avoids redundant test runs across .NET 8/9/10 which share the same code paths.
• Pinned action versions (checkout@v7, setup-dotnet@v6, upload-artifact@v7, download-artifact@v8, codecov-action@v6) are newer majors than commonly seen — worth double-checking these tags exist/are stable at merge time if CI hasn't actually run green on them yet (the PR description says it has, so likely fine).

Overall: well-scoped, mechanical, and the incidental bug fixes are a nice bonus from actually exercising the net48 path. No blocking issues found.

[claude]

Review

This is a CI/build infrastructure migration (Azure Pipelines → GitHub Actions) with no new quantities or units. Overall it's a well-executed, carefully-scoped migration — nice attention to security and cross-platform detail. A few notes:

Breaking changes

None for library consumers. UnitsNet.Serialization.JsonNet, UnitsNet.NumberExtensions, and UnitsNet.NumberExtensions.CS14 now embed README.md (PackageReadmeFile), matching what UnitsNet.csproj already does — a nice-to-have, not a breaking change.

Changes to generated code

CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs makes one mechanical substitution: Enum.GetValues<T>() → EnumHelper.GetValues<T>() in the HasAtLeastOneAbbreviationSpecified test. I spot-checked the regenerated output across LengthTestsBase.g.cs (ILinearQuantity), TemperatureTestsBase.g.cs (IAffineQuantity), and LevelTestsBase.g.cs (ILogarithmicQuantity) — the diff is identical and consistent in all three (and by extension the other ~125 regenerated files). EnumHelper.GetValues<T> (UnitsNet/InternalHelpers/EnumHelper.cs) is pre-existing and correctly falls back to Enum.GetValues(typeof(T)).Cast<T>() before NET7_0_OR_GREATER, since the generic Enum.GetValues<T>() overload isn't available on netstandard2.0/net48. This is exactly the right fix to unblock the new CLR4/net48 compatibility suite.

Code quality / correctness fixes bundled in

A few real bug fixes ride along with the CI migration, since they were required to get the net48 suite green:

• UnitsNet/Extensions/LogarithmicQuantityExtensions.cs: the netstandard2.0 RootN fallback previously did Math.Pow(number, 1.0/n), which returns NaN for a negative base with an odd root (e.g. cube root of -8). The fix special-cases negative numbers correctly. Good catch. Minor nit: the test file (LogarithmicQuantityExtensionsTest.cs) duplicates this exact algorithm in a local RootN helper instead of asserting against an independently-computed expected value — it verifies the test compiles/matches the impl rather than validating correctness (pre-existing pattern via double.RootN on NET, so not a regression, just worth knowing it's not an independent check).
• UnitsNet.Tests/CustomQuantities/HowMuch.cs: updates the #if !NET explicit interface implementation from IQuantity.QuantityInfo to IQuantityOfType<HowMuch>.QuantityInfo, matching the interface shape used elsewhere (e.g. generated quantities). This was presumably a latent compile error on netstandard2.0/net48 that just wasn't being built/tested before.
• Null-argument assertions in the JSON.NET serialization tests now check exception.ParamName instead of exception.Message — correct, since ArgumentNullException.Message text differs between .NET Framework and modern .NET, and ParamName is the stable contract.

These are reasonable to bundle since they're purely enablers for the new CI target, not unrelated scope creep.

CI/workflow specifics

• Good security hygiene: Codecov now authenticates via short-lived OIDC (use_oidc: true) instead of a stored CODECOV_TOKEN; NUGET_ORG_APIKEY is passed via env: and referenced as $NUGET_ORG_APIKEY rather than interpolated directly into the run: string, avoiding secret leakage into shell history/process args; explicit permissions: blocks scope tokens per job.
• pr.yml correctly swaps EnricoMi/publish-unit-test-result-action/windows@v2 → the non-Windows variant now that the runner moved from windows-latest to ubuntu-latest — easy to miss, glad it was caught.
• One thing worth double-checking: ci.yml's concurrency: { group: ci-..., cancel-in-progress: true } applies to the whole workflow, which includes publish-nuget. If two pushes to master land close together, an in-flight NuGet publish could theoretically be cancelled mid-push by a newer run. --skip-duplicate mitigates a lot of the risk, but you may want cancel-in-progress: false (or a separate concurrency group) for the publish job specifically, since builds are cheap to cancel but publishes aren't.
• net48-compatibility.yml only triggers on push to master (pull_request is commented out) and can't be dispatched pre-merge on a net-new workflow file, so the first real signal comes after merging to master. That's clearly documented in the PR description and GITHUB_ACTIONS_MIGRATION_README.md, and the author validated locally, so it's a reasonable, acknowledged tradeoff rather than an oversight — just flagging it as the one place a regression could reach master before being caught.
• Action versions bumped fairly far ahead (actions/checkout@v7, actions/setup-dotnet@v6, actions/upload-artifact@v7, actions/download-artifact@v8, codecov/codecov-action@v6) — assuming these are verified to exist and were pinned intentionally (the PR's linked CI runs suggest so), just worth a sanity check that they're pinned to a real released tag and not a typo'd future version.

Build scripts

Build/build-functions.psm1 / build.ps1 / init.ps1 correctly switch from hardcoded \-separated Windows paths to Join-Path, and pick reportgenerator vs reportgenerator.exe based on OSVersion.Platform — necessary and correctly done for the new Linux runners.

Test coverage

Reasonable trade-off: full suite + coverage only runs on net10.0 in the main pipeline (net8/9 covered by build-only), with the CLR4/netstandard2.0 path fully tested separately post-merge. Documented rationale (net8.0, net9.0, net10.0 exercise the same code paths) is sound.

No security concerns beyond the OIDC/secret-handling improvements noted above (which are net positives).