Skip to content

Describe UnitsNet.Modular numeric format strings - #1716

Merged
angularsen merged 1 commit into
masterfrom
agl-codex/modular-string-syntax
Aug 1, 2026
Merged

Describe UnitsNet.Modular numeric format strings#1716
angularsen merged 1 commit into
masterfrom
agl-codex/modular-string-syntax

Conversation

@angularsen

Copy link
Copy Markdown
Owner

Motivation

UnitsNet.Modular forwards quantity format strings such as F1 to the stored numeric value, but its public APIs do not currently identify those parameters as numeric format strings to IDE tooling.

Changes

  • annotate generated quantity ToString(string) overloads with StringSyntaxAttribute.NumericFormat
  • annotate the type-erased IQuantityDescriptor.Format format parameter
  • verify the annotations are present in emitted consumer-visible metadata

This improves validation and completion for direct format-string arguments where supported by the IDE. Interpolation clauses such as $"{speed:F1}" continue to work, but their completion behavior remains controlled by the IDE's interpolated-string support.

Validation

  • dotnet test UnitsNet.Modular/UnitsNet.Modular.slnx --no-restore -m:1 -p:UnitsNetModularSampleUpdateLocalPackagesOnBuild=false
  • 115 tests passed
  • Modular runtime built for .NET 8, .NET 9, and .NET 10

@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review

Small, well-scoped PR — adds [StringSyntax(StringSyntaxAttribute.NumericFormat)] to the format-string parameters on generated ToString(string?)/ToString(string?, IFormatProvider?) and on IQuantityDescriptor.Format, purely to improve IDE completion/validation. No behavior change.

Breaking changes: None. This is purely additive metadata (an attribute on existing parameters), and UnitsNet.Modular is still pre-1.0 (6.0.0-alpha.0 per UnitsNet.Modular.Versioning.props), so there'd be no compat concern even if it were.

Scope is well thought through:

  • The internal UnitsNet.Modular.SourceGen.QuantityOperations.Format helper is correctly left unannotated — it's [EditorBrowsable(EditorBrowsableState.Never)] generator infrastructure, not a consumer-facing surface.
  • QuantityInfo's explicit IQuantityDescriptor.Format implementation is correctly left unannotated too, since explicit interface implementations pick up attribute-driven tooling from the interface declaration, not the implementation.
  • One inherent gap worth knowing about (not a request to fix): IQuantity<TValue> implements System.IFormattable, whose ToString(string?, IFormatProvider?) can't be annotated since it's a BCL interface. So the hint only shows up when calling through the concrete generated struct, not when code goes through IFormattable/IQuantity<T>. That's an unavoidable limitation of StringSyntaxAttribute, not something this PR should try to work around.

Style/conventions: Matches the surrounding QuantityEmitter.cs code — the constant name, global::-qualified attribute string, and multi-line writer.AppendLine/.Append emission are consistent with how the rest of the file builds up source text. The unqualified (non-global::) attribute usage in QuantityDescriptor.cs also matches other attributes already used in that file (e.g. NotNullWhen).

Test coverage: FormattingApis_IdentifyNumericFormatSyntax reflects over Length.ToString(string) and IQuantityDescriptor.Format and asserts the attribute is present with the right Syntax value — a reasonable smoke test proving the attribute survives codegen and interface declaration. Only one quantity is checked, but since the attribute is emitted unconditionally for every generated quantity (not conditioned on IsLogarithmic/IsAffine), that's sufficient — no need to duplicate the check per quantity kind.

Minor observation (optional, not blocking): The main UnitsNet library's own ToString(string) overloads (e.g. Length, QuantityValue) don't carry this attribute yet, so there's now a small inconsistency between the two codebases' formatting APIs. Out of scope for this PR, but could be a good follow-up if you want the same IDE completion in the main library.

Overall: clean, minimal, and correctly scoped change with a test verifying the actual emitted/declared metadata rather than just the generator's string output.

@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 (5aad11f) to head (fb44ad2).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1716     +/-   ##
========================================
- Coverage      98%     98%     -1%     
========================================
  Files         512     514      +2     
  Lines       22987   24141   +1154     
========================================
+ Hits        22715   23748   +1033     
- Misses        272     393    +121     
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.

@angularsen
angularsen force-pushed the agl-codex/modular-string-syntax branch from f4f7d8b to fb44ad2 Compare August 1, 2026 20:56
@angularsen
angularsen marked this pull request as ready for review August 1, 2026 20:56
@angularsen
angularsen merged commit bce623e into master Aug 1, 2026
4 checks passed
@angularsen
angularsen deleted the agl-codex/modular-string-syntax branch August 1, 2026 20:56
@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review

Reviewed the diff (UnitsNet.Modular/UnitsNet.Modular.Generator/QuantityEmitter.cs, UnitsNet.Modular/UnitsNet.Modular/Metadata/QuantityDescriptor.cs, and the added test). Overall this is a small, well-scoped, additive change — nice work.

Breaking changes

None. Adding [StringSyntax(StringSyntaxAttribute.NumericFormat)] to existing parameters is purely metadata — no change to the public API shape (source- or binary-compatible). UnitsNet.Modular also ships as alpha via MinVer, so even a breaking change here would be low-risk, but this isn't one anyway.

Scope note

This only touches UnitsNet.Modular (the newer, source-generator-based sibling project), not the main UnitsNet/CodeGen pipeline described in AGENTS.md. The generated Length.ToString(string) etc. in the main library are unaffected, so there's now a minor inconsistency between the two code generators (main CodeGen doesn't emit StringSyntax on quantity ToString(string) overloads). Not a blocker, but worth a follow-up issue if you want parity across both generators eventually.

Generated code changes

  • QuantityEmitter.cs: the new NumericFormatStringSyntaxAttribute constant is emitted before both the one-arg and two-arg ToString overloads, splitting each signature across multiple AppendLine/Append calls. This matches the existing raw-StringBuilder emission style used throughout the file (no template engine), and the multi-line parameter formatting is consistent with how other multi-parameter members are declared in the surrounding hand-written code (e.g. IQuantityDescriptor in QuantityDescriptor.cs).
  • The fully-qualified global::System.Diagnostics.CodeAnalysis.StringSyntax(...) avoids any dependency on using directives in the generated file — good, since the emitter otherwise fully-qualifies everything else too.
  • QuantityDescriptor.cs: IQuantityDescriptor.Format's format parameter (index 1) is annotated directly in hand-written code — correct target, verified against the test.
  • Consistency check: UnitsNet.Modular.SourceGen.QuantityOperations.Format and the explicit IQuantityDescriptor.Format implementation in QuantityInfo<TQuantity,TUnit> were not annotated, but that's correct — per ARCHITECTURE.md, SourceGen.* types are internal generator plumbing ([EditorBrowsable(Never)]) not meant for direct consumer use, and the explicit interface implementation is only reachable through the (now-annotated) interface reference.
  • IQuantity<TValue> extends IFormattable, whose inherited ToString(string?, IFormatProvider?) signature isn't annotated (can't be — it's a BCL interface). That's an inherent limitation, not something this PR should try to solve, but worth knowing that the analyzer hint only applies when calling ToString on the concrete generated struct type directly (the common case), not via an IFormattable/IQuantity reference.

Test coverage

FormattingApis_IdentifyNumericFormatSyntax reflects over Length.ToString(string) and IQuantityDescriptor.Format and asserts the attribute + NumericFormat syntax value are present. Since ToString emission isn't branched by quantity kind (logarithmic/affine/linear all share the same emitted method), one representative type is sufficient — no need to test all three ILinearQuantity/IAffineQuantity/ILogarithmicQuantity variants here.

Minor nit

NumericFormatStringSyntaxAttribute as a constant name is a little misleading since it holds the full attribute usage text (StringSyntax(...)), not just an attribute type name — something like NumericFormatStringSyntaxUsage would read more precisely. Not blocking.

Other

  • No new quantities/units, no security-relevant surface.
  • Docs (motivation/validation) in the PR description are clear and the manual validation steps (115 tests, multi-TFM build) look sufficient for a change this size.

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