WIP: ElectricAcDc #1312
WIP: ElectricAcDc #1312
Conversation
…ertia, and 'slug' as a unit of Mass
# Conflicts: # Common/GeneratedCode/Quantities/Mass.Common.g.cs # Common/GeneratedCode/Quantities/MassMomentOfInertia.Common.g.cs # Common/UnitDefinitions/Mass.json # Common/UnitDefinitions/MassMomentOfInertia.json # UnitsNet/GeneratedCode/Extensions/Number/NumberToMassExtensions.g.cs # UnitsNet/GeneratedCode/Extensions/Number/NumberToMassMomentOfInertiaExtensions.g.cs # UnitsNet/GeneratedCode/Quantities/Mass.NetFramework.g.cs # UnitsNet/GeneratedCode/Quantities/MassMomentOfInertia.NetFramework.g.cs
# Conflicts: # UnitsNet.Tests/CustomCode/ParseTests.cs
There was a problem hiding this comment.
It would helpful if you could summarize the changes and what the changes aim to accomplish in the PR description.
On the surface, this looks useful enough.
However, I don't know this domain well enough to give any meaningful comments on the design of the wrapper types. It's just one those things that someone who actually needs it will have to start using and massage until it solves several use cases.
It is important for me to only add something to UnitsNet that will actually be used and solves a need that many UnitsNet consumers will benefit from.
So to be very direct 😄
Do you plan to use this yourself?
Will it solve a real need for you?
Are there any known unknowns, things to still explore or figure out in the design or use cases to test the design against?
Do you know other developers working in this domain that we could involve in this discussion?
It doesn't have to be perfect on the first go, but it is helpful to think along these lines from the start.
| } | ||
|
|
||
| /// <summary> | ||
| /// Converts <see cref="UnitsNet.Pressure.Value" /> to <see cref="double" /> at <see cref="ElectricCurrentFlowReference" /> |
There was a problem hiding this comment.
Some old references to pressure here and there, but you can fix these things later when the design is more complete.
| { | ||
| if (Current.Value < 0) | ||
| { | ||
| throw new ArgumentOutOfRangeException(nameof(Current), "Direct or RMS current cannot be less than zero."); |
There was a problem hiding this comment.
If we enforce this in the constructor instead, then it will fail earlier and it seems like none of the reference types allow negative current value.
| /// </summary> | ||
| /// <param name="reference">The <see cref="ElectricCurrentFlowReference" /> to convert <see cref="ElectricCurrentFlow" /> to.</param> | ||
| /// <returns>The value of pressure at <see cref="ElectricCurrentFlowReference" /></returns> | ||
| private double AsBaseNumericType(ElectricCurrentFlowReference reference) |
There was a problem hiding this comment.
I know you just based it on the existing PressureReference type, but the name AsBaseNumericType does not tell me much.
To be more consistent with IQuantity, maybe wrapper types could follow this:
double As(reference), similar todouble IQuantity.As(unit)ElectricCurrentFlow ToReference(reference), similar toIQuantity.ToUnit(unit).ToReference(reference)also allows you to dodouble current = myCurrentFlow.ToReference(myReference).Current.Valuesimilar todouble value = myIQuantity.ToUnit(myUnit).Value;
The only minor gotcha is Power is using decimal as its numeric value type, but we already intend to change this to double in #1195.
| @@ -1,20 +0,0 @@ | |||
| { | |||
| "Name": "ElectricPotentialAc", | |||
There was a problem hiding this comment.
Just a heads up that we can't delete these, we have to mark them obsolete in JSON with ObsoleteText on the quantity for example just below Name on line 3, then add to #1200 , then remove them in next major version.
The obsolete text should describe what they should use instead for AC and DC operations.
This can wait until the design is more complete of course.
|
If I understand correctly, we concluded to not continue with this PR? |
This is a rough cut at the reference type suggested in #1296. Primarily for review and to see if I'm on the right track or not.