Skip to content

Adding method to convert quantity into another quantity #457

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 2, 2018

Conversation

tmilnthorp
Copy link
Collaborator

No description provided.

@tmilnthorp
Copy link
Collaborator Author

I wonder if we should obsolete the ToString overloads that take a unit?

For example string1 and string2 should be the same here:

            var someAcceleration = Acceleration.FromMetersPerSecondSquared( 9.8 );
            var someAccelerationInInchesPerSecondSquared = someAcceleration.AsQuantity( AccelerationUnit.InchPerSecondSquared );

            var string1 = someAcceleration.ToString( AccelerationUnit.InchPerSecondSquared );
            var string2 = someAccelerationInInchesPerSecondSquared.ToString();

It makes the fact you're doing a conversion explicit, and ToString is responsible only for how that particular unit is displayed - not converting and displayed.

It would also make ToString available from an interface.

@angularsen
Copy link
Owner

angularsen commented Jun 27, 2018

It makes the fact you're doing a conversion explicit, and ToString is responsible only for how that particular unit is displayed - not converting and displayed.

  1. I see your point, but I would counter with that this type of conversion+formatting is already widely used in .NET in all the date and time types so I would not want to obsolete it. I find it intuitive to be able to do it this way and the syntax is a bit shorter too.

  2. How about renaming it to AsUnit(Unit) or WithUnit(Unit)? I did not immediately get the semantics of AsQuantity(Unit) before looking at the explanation. The WithXX() naming convention is widely used for immutable types, but it doesn't perhaps indicate that you are also changing the value, so I think I prefer AsUnit().

  3. Instead of generating N sets of tests for N quantities I think it's better to handpick a couple of quantities instead. We have done N tests in the past, but the test suite is unnecessarily large because of it. Since we know we are generating the code I think it's more valuable to test quantities with actual differences:
    3.1 Information for decimal type
    3.2 Length or some other with double type
    3.3 PowerRatio with logarithmic units and different code generation

@tmilnthorp
Copy link
Collaborator Author

tmilnthorp commented Jun 27, 2018

  1. True! I'm mostly ok with that then. However, all of the DateTime.ToString methods simply take a format string - not any sort of conversion enum. Generally if a ToString method does some sort of conversion, the conversion is specified by the format string - not an enumeration.

  2. I'm actually more used to seeing ToXXX for conversion methods (Object.ToString, Convert.ToDouble, DateTime.ToBinary, etc.). I was just trying to follow the existing pattern in Units.NET in the AsXXX method naming. Since we call Acceleration/Power/etc a quantity (especially important if/when we have an IQuantity interface), I would expect ToQuantity() here (AsQuantity in Units.NET speak). Saying AsUnit seems ambiguous compared to the As( Unit ) method (which I think would be more clear as ToDouble(Unit), but that's just me).

  3. I was erring on the side of caution. I I like to test every public API, even if all it does is call other methods. These are covered by the As tests as well as constructor tests, so it's not entirely necessary to do it again. They could be removed entirely. I'll leave it up to you!

@angularsen
Copy link
Owner

  1. Good point, I guess it is not an apple to apple comparison even if I there are some similarities. I still think we can keep them around as I find it more intuitive/discoverable and shorter.

  2. I agree with all you say here. I think perhaps I prefer Length ToUnit(LengthUnit) for this PR and later extended with double ToDouble(LengthUnit) or possibly QuantityValue ToValue(LengthUnit) and then obsoleting As(LengthUnit)?

  3. My initial knee jerk reaction is that generating tests for generated code gives little added confidence about the code, but the counter argument is that then at least we could use test coverage tools in a meaningful way so I think I just changed my mind on this. Let's keep the tests 🤔😄

@tmilnthorp
Copy link
Collaborator Author

We'll think about point 1 for another PR :)

I think that sounds good - I've renamed this method ToUnit. We could address the other methods later.

@angularsen angularsen merged commit fe89064 into angularsen:master Jul 2, 2018
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.

2 participants