Remove double pseudo-quantity from relations#1679
Conversation
|
@angularsen Why |
|
Sorry no, the AI got a bit eager here. Reverted. |
angularsen
left a comment
There was a problem hiding this comment.
After looking at the resulting API surface, I’m not sure this is an improvement as-is.
My main concern is that this seems to solve a maintainer/codegen concern more than a user need. Removing double as a pseudo-quantity simplifies the generator model, but it changes the public API from a direct dimensionless double result to Scalar, which feels less ergonomic and more surprising for callers.
Concretely, this is a breaking change:
double value = specificEnergy * brakeSpecificFuelConsumption;now becomes something like:
double value = (specificEnergy * brakeSpecificFuelConsumption).Amount;and inverse operations also become less natural:
var bsfc = Scalar.FromAmount(2) / specificEnergy;instead of:
var bsfc = 2 / specificEnergy;I’m also hesitant to make Scalar carry more of the relation system. With this change, Scalar starts implementing strongly typed operator interfaces for specific quantity combinations, for example division by SpecificEnergy or BrakeSpecificFuelConsumption. That may be technically consistent, but it means every future dimensionless relation could add more generated operators to Scalar.
Scalar itself also feels a bit awkward as an IQuantity. It has APIs like:
double value = scalar.As(ScalarUnit.Amount);and Scalar / Scalar returns double, so it still leaks back out of the quantity model. It also has very little semantic use in the codebase today, so this PR would make it more central without first settling what role Scalar should really have.
Given that Scalar is semantically close to “just a value”, and QuantityValue may become more capable in #1544, I think we should be careful about expanding Scalar before we have a clearer design for dimensionless results.
So my current preference is that we don’t merge this as-is. I think we should either:
- keep
doublefor dimensionless relation results and accept the generator special case, - remove this generated relation if it is not important enough to justify the special case,
- or first agree on a broader design for dimensionless quantities/values before changing this API.
|
I agree with your observations, lets drop this for now. How about getting rid of |
|
Yes, it seems Scalar is largely unused in our code base, but looking at the history of why it was added in the first place then it seems to help consumers consolidate logic for "counts" vs "quantities". I'm thinking it may cause more harm removing it than just keeping it around, as long as we are consistent in how use it - or rather, not use it, in our APIs. At any rate, the draft PRs are up #1680 and #1681. We can discuss further in 1680. |
Summary
Removes
doubleas a pseudo-quantity from relation generation so generated relations stay between UnitsNet quantities, with inverse relations still represented by1.The remaining dimensionless relation is represented as a scalar factor:
Scalar.Amount = SpecificEnergy.JoulePerKilogram * BrakeSpecificFuelConsumption.KilogramPerJouleThis follows the discussion that the direct product is not an efficiency ratio. It is a plain energy/fuel-consumption factor; an efficiency ratio would require inverting this result.
Maintainer Updates
Scalar.Amountafter review of the linked v6 discussion.Scalar / SpecificEnergyandScalar / BrakeSpecificFuelConsumption....EqualsEnergyto...EqualsEnergyFactor.Validation
generate-code.bat.dotnet test UnitsNet.Tests --filter "FullyQualifiedName~BrakeSpecificFuelConsumptionTests|FullyQualifiedName~SpecificEnergyTests|FullyQualifiedName~ScalarTests" --no-restore --verbosity minimal.