Add implicit cast to double for Scalar.

[CallumECameron]

Fixes #897
Allows Scalar to be used in arithmetic operations with other quantities #897 .

[CallumECameron]

There are two failing tests, Convert_ToBool_ThrowsInvalidCastException and Convert_ToString_EqualsToString. This is because Convert.ToString and Convert.ToBool are using the double overloads rather than the object overloads.

Ideally, Scalar would behaviour like double for arithmetic operations but not for other behaviours. Anyone have any thoughts here?

[angularsen]

Ah, tough one. I'm not sure that is possible.

I think we are left with these options then:

1. Don't change anything, use myMass * myScalar.Value or similar - explicitly getting the double value when needed.
2. Add Scalar overload for every arithmetic method. I think it'll be a lot of code, and something we would have to remember keeping in sync. Not sure I favor this one.

[tmilnthorp]

If an object is implicitly convertible to a type, it can be used implicitly as a substitute for that type in any context or method overload. That beauty is the entire point of the PR, so I would expect this behavior.

If you want the object methods anywhere an implicit conversion is possible, you now have to be explicit:

[Fact]
public void Convert_ToBool_ThrowsInvalidCastException()
{
    var quantity = Scalar.FromAmount(1.0);
    Assert.Throws<InvalidCastException>(() => Convert.ToBoolean((object)quantity));
}

Anything that is not a Scalar left-hand-side would also work:

[Fact]
public void Convert_ToBool_ThrowsInvalidCastException()
{
    IConvertible quantity = Scalar.FromAmount(1.0);
    Assert.Throws<InvalidCastException>(() => Convert.ToBoolean(quantity));
}

[Fact]
public void Convert_ToBool_ThrowsInvalidCastException()
{
    IQuantity quantity = Scalar.FromAmount(1.0);
    Assert.Throws<InvalidCastException>(() => Convert.ToBoolean(quantity));
}

[angularsen]

Good point, I was just assuming we caused problems by breaking the IConvertible tests, but as you point out we should be able to think of Scalar as a double. So maybe just fixing support for passing these two generated tests is all that it takes.

[tmilnthorp]

As the test points out, it is a breaking behavior change, albeit a useful one.

[angularsen]

I don't consider it a breaking change.

1. The fact that Scalar would previously result in InvalidCastException from Scalar to object when calling Convert.ToBoolean(Scalar.FromAmount(1)) is not something I consider a feature or by design.
2. Convert_ToString_EqualsToString actually found a bug in UnitsNet.QuantityFormatter.Format where we did not trim the string, so quantities like Scalar and Ratio without a unit abbreviation would have a trailing whitespace. Fixed just now in Trim ToString() for units without abbreviation #912.

[tmilnthorp]

I would disagree. Even if not by design, it is a break in the behavior. For example, even a call to Equals that may have previously returned false may now return true:

double value = 3.0;
var scalar = Scalar.FromAmount( value );

var areEqual = value.Equals( scalar );   // Currently false. Will now be true.

You never know what clients are assuming. Maybe I've been hurt too much 😆

That said I do not think that this should hold this back. However I do think we would also want to do the reverse: allow implicit conversion from double to Scalar. That would also allow us to take advantage of the operator overloads for Scalar math.

Scalar * 3.0 for example.

[angularsen]

Implicit cast the other way would be nice too, I think. Can't see any problems with it immediately. So, it seems we agree we can move forward with this minor breaking change then. The generated tests would need to be adapted to make an exception for Scalar for this one failing test. The simplest option I can think of right now is to make an exception in the codegen for Scalar, to skip this test boolean test case.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[cedricmoers]

I would be very much interested in this as well, will this become part of the master branch any time soon?

[angularsen]

@cedricmoers it seems this was closed due to inactivity, would you be interested in picking up and completing the PR?

[cedricmoers]

I would love to help out and finish this ticket, but I fear I am not quite familiar in GIT to do so. I am quite interested to learn though. Could you provide me a basic description in what you would expect me to do? This way I can look up by myself on how to do this.

[angularsen]

I'm happy to assist.

1. Fork the github repo to get your own copy
2. Clone your fork's github repo to a folder on your machine
3. Make sure you are on master branch, then create and checkout a new git branch, f.ex. implicit-cast-scalar (git branch, git checkout)
4. Copy in the changes from this PR (not many lines, so easiest to just copy & paste), (git add, git commit)
5. Push your branch to your fork's github repo (git push)
6. Create pull request from your forks' github website

I recommend a visual git tool to help you out, command line can be daunting to get started with.
Several free ones exist, I did like SourceTree a while ago. Github Desktop is supposedly also easy to get started with, but haven't tried it.

I prefer Fork, but it is commercial.

[cedricmoers]

Thank you for the clear explanation. I made a pull request where I added the implicit cast and the exception for the unit test: #977

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.