Add NominalPower#1644
Conversation
angularsen
left a comment
There was a problem hiding this comment.
Thanks for the contribution! A couple of things need to be fixed in NominalPower.json:
1. Missing BaseDimensions
NominalPower is dimensionally equivalent to Power (W = kg·m²/s³). The quantity definition needs BaseDimensions, same as Power.json:
"BaseDimensions": {
"L": 2,
"M": 1,
"T": -3
}2. Missing BaseUnits on WattPeak
The WattPeak unit needs BaseUnits to map to SI base units, same as Power.json's Watt:
"BaseUnits": {
"L": "Meter",
"M": "Kilogram",
"T": "Second"
}Please add these and the PR should be good to go!
| private void GenerateConversionMethods() | ||
| { | ||
| Writer.WL($@" | ||
| #region Conversion Methods |
There was a problem hiding this comment.
Please don't change generator code unless intentional
| @@ -0,0 +1,25 @@ | |||
| { | |||
| "Name": "NominalPower", | |||
There was a problem hiding this comment.
Should we rename this to disambiguate this from other kinds of nominal power?
It seems maybe a bit ambiguous: https://en.wikipedia.org/wiki/Nominal_power
For example, NominalPowerPhotovoltaic
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
Code ReviewAdding Conceptual concern — is NominalPower a distinct quantity?
Adding a separate quantity for this is unusual. The library currently avoids quantities that are just "power measured in a specific context." Could this instead be handled as additional units in the existing If there is a reason they must stay separate (e.g., preventing accidental addition of peak-watts and thermal-watts), please explain it in the PR description. Code generator change bundled in same PRThe large diff (~12k additions / ~9k deletions) is mostly generated code regenerated after fixing indentation in Unit definition
Recommendation
|
Based on New Unit added: Nominal Power by @McGum