Skip to content

Add conversion properties that return a quantity #1051

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

Closed
wants to merge 3 commits into from

Conversation

tmilnthorp
Copy link
Collaborator

@tmilnthorp tmilnthorp commented Feb 17, 2022

  • Adding conversion properties that return a quantity
  • Obsolete conversion properties that return only a double

This allows you to always get back a quantity, then reuse that quantity for other operations.

For example to calculate Mass and you'd use the following method:

public static Mass operator *(Density density, Volume volume)

This allows for nicer conversion

Density density = Density.FromKilogramsPerCubicMeter( 3.0 );
Volume volume = Volume.FromLiters( 3.0 );

// Current method
Mass mass = density.ToUnit(DensityUnit.CentigramPerLiter) * volume.ToUnit(VolumeUnit.CubicFoot);

// With this PR
Mass mass = density.ToCentigramsPerLiter * volume.ToCubicFeet;

It also lets you care less about if Value is a double or decimal. If you never need to get the Value, it keeps you "in" a UnitsNet type.

I figured I'd throw this up here now, since it would be nice to remove the current conversion properties (that assume double, even for a decimal value), as it will bloat the binary size temporarily with 2x conversion properties.

@tmilnthorp
Copy link
Collaborator Author

Downloading Tools/7-zip/7za.exe (536 KB)
Error downloading object: Tools/7-zip/7za.exe (fa252e5): Smudge error: Error downloading Tools/7-zip/7za.exe (fa252e501332b7[4](https://ci.appveyor.com/project/angularsen/unitsnet/builds/42619991#L4)86a972e7e471cf6915daa681af35c6aa102213921093eb2a3): batch response: This repository is over its data quota. Account responsible for LFS bandwidth should purchase more data packs to restore access.

Uh oh.

@tmilnthorp
Copy link
Collaborator Author

This is ready, but getting a mysterious build error.

MSBUILD : error MSB4017: The build stopped unexpectedly because of an unexpected logger failure.
Microsoft.Build.Exceptions.InternalLoggerException: The build stopped unexpectedly because of an unexpected logger failure.
 ---> System.Net.WebException: The remote server returned an error: (500) Internal Server Error.
   at System.Net.HttpWebRequest.GetResponse()
   at System.Net.WebClient.GetWebResponse(WebRequest request)
   at System.Net.WebClient.DownloadBits(WebRequest request, Stream writeStream)
   at System.Net.WebClient.UploadBits(WebRequest request, Stream readStream, Byte[] buffer, Int32 chunkSize, Byte[] header, Byte[] footer)
   at System.Net.WebClient.UploadDataInternal(Uri address, String method, Byte[] data, WebRequest& request)
   at System.Net.WebClient.UploadData(Uri address, String method, Byte[] data)
   at AppVeyor.BuildAgent.Api.RestBuildServices.AddCompilationMessage(String message, Nullable`1 category, String details, String fileName, Nullable`1 line, Nullable`1 column, String projectName, String projectFileName)
   at AppVeyor.MSBuildLogger.AppVeyorLogger.SendCompilationMessage(BuildMessageCategory category, String message, String code, String file, Int32 lineNumber, Int32 columnNumber, Int32 projectId)
   at AppVeyor.MSBuildLogger.AppVeyorLogger.eventSource_WarningRaised(Object sender, BuildWarningEventArgs e)
   at Microsoft.Build.BackEnd.Logging.EventSourceSink.RaiseWarningEvent(Object sender, BuildWarningEventArgs buildEvent)
   --- End of inner exception stack trace ---
   at Microsoft.Build.Exceptions.InternalLoggerException.Throw(Exception innerException, BuildEventArgs e, String messageResourceName, Boolean initializationException, String[] messageArgs)
   at Microsoft.Build.BackEnd.Logging.EventSourceSink.RaiseWarningEvent(Object sender, BuildWarningEventArgs buildEvent)
   at Microsoft.Build.BackEnd.Logging.EventSourceSink.RaiseAnyEvent(Object sender, BuildEventArgs buildEvent)
Build failed with exit code 1.
Command exited with code 

@Muximize
Copy link
Contributor

I don't really understand the benefit of these conversion properties. The whole point of UnitsNet is to not care too much about the underlying values, right?

So instead of your current method (which works, but is a bit verbose) why not just do:

Mass mass = density * volume;

And to get a raw value out at some later point:

double masskg = mass.Kilograms;

That is my preferred way of using UnitsNet, it makes the unit explicit when extracting a raw value. But it would be deprecated by this PR! 😧

I try to avoid using .Value, I'd rather see that deprecated. It enables the conversion and value extraction to be separate, which can be confusing:

Mass othermass = mass.ToKilograms;
...
...
...
double massgrams = othermass.Value; // Surely it was grams, right?

@angularsen
Copy link
Owner

angularsen commented Feb 25, 2022

At first glance, I share the gut feeling of @Muximize .

Adding .ToGrams() provides a minor syntax improvement over ToUnit(MassUnit.Gram).
Removing .Grams loses out on a lot of the static type safety I think UnitsNet brings.

Due to 1000+ units and binary size issues, I don't want to have both .ToGrams() and .Grams, so I think we would have to pick one.

The problem; if you take a Mass parameter into a method, you should not need to know what unit it was constructed with. You might as well have passed double. Or am I misunderstanding this PR?

@tmilnthorp
Copy link
Collaborator Author

The whole point of UnitsNet is to not care too much about the underlying values, right?

I think you're agreeing with me actually 😄 Encapsulation is important. These properties allow you to do conversion to a different unit and still keep encapsulation in a UnitsNet quantity as the returned type. Otherwise it's just a double (even if the quantity was not a double originally such as BitRate). It forces you to not care about the underlying value when asking for a quantity in a different unit.

Currently:

Volume volume = Volume.FromLiters( 3.0 );

// Surely it was cubic feet, right?   ;)
double volumeInCubicFeet = volume.CubicFeet;

Is now:

Volume volume = Volume.FromLiters( 3.0 );

// Definitely in cubic feet   ;)
Volume volumeInCubicFeet = volume.ToCubicFeet;

@tmilnthorp
Copy link
Collaborator Author

Due to 1000+ units and binary size issues, I don't want to have both .ToGrams() and .Grams, so I think we would have to pick one.

This is to propose a move to this style and remove the double versions at v5. But if you just change it, it's a major breaking change with no obsolete message.

The problem; if you take a Mass parameter into a method, you should not need to know what unit it was constructed with. You might as well have passed double. Or am I misunderstanding this PR?

Perhaps that was a bad example. Let's go super simple: I have a length that I want to display in meters.

private static string GetStringInMeters(Length length)
{
	return inches.Meters; // Can't do currently: just a double      
	return inches.ToMeters.ToString(); // Now it's easy! It's in a UnitNet type already
}

Currently your only option is:

private static string GetStringInMeters(Length length)
{
	return inches.ToUnit(LengthUnit.Meter).ToString();
}

And getting the value is easy if you so choose:

private static double GetLengthInMeters(Length length)
{
	return inches.ToMeters.Value;
}

What's the point of the handy conversion properties if using them loses the power of UnitsNet?

@angularsen
Copy link
Owner

angularsen commented Feb 25, 2022

I think it comes down to what the primary use case is.
Both solutions work, I believe they just optimize syntax for one way or the other.

  1. inches.ToMeters syntax is optimized to get a new quantity in the given unit
  2. inches.Meters syntax is optimized to get the double in the given unit.

Both are more cumbersome to get the other representation.

I honestly don't know what I would consider the primary use case, and I am probably colored by many years of seeing .Kilograms properties and expecting double values out. It does more closely resemble TimeSpan and feels familiar in that regard.

@Muximize
Copy link
Contributor

Muximize commented Feb 26, 2022

Encapsulation is important. These properties allow you to do conversion to a different unit and still keep encapsulation in a UnitsNet quantity as the returned type.

But this way the encapsulation is broken. In my opinion, a quantity having a unit this quantity was constructed with is an implementation detail I don't want to deal with. UnitsNet quantities already are leaky abstractions, and this PR would make that worse.

The way I use UnitsNet, there are entry points where external inputs enter the system and get "wrapped" in a quantity, eg. Power.FromKilowatts(input). From there, the unit doesn't matter anymore. Then they get processed, calculated with, and at some point turned into outputs of the same or a different quantity in a certain unit, eg. power.Watts or energy.Joules.

These entry and exit points are the only places where I care about conversion. That is the whole point of the encapsulation, that any Power or other quantity is equal to any other of the same type and ideally does not have magic internal state that makes them behave differently when you call .Value or .ToString().

I think the static type safety and usability of UnitsNet would improve by deprecating .Value and .ToUnit() and making the internal representation the implementation detail it should be.

As for the string situation, would this be an option?

private static string GetStringInMeters(Length length)
{
	return length.ToString(); // Without arguments always uses BaseUnit
}

private static string GetStringInInches(Length length)
{
	return length.ToString(LengthUnit.Inch);
}

What's the point of the handy conversion properties if using them loses the power of UnitsNet?

The power of UnitsNet is only having to deal with conversion at the inputs and outputs, the edges of a system.

@angularsen
Copy link
Owner

angularsen commented Feb 26, 2022

I agree with your sentiment @Muximize, that in my use of UnitsNet I generally put values in, maybe do some arithmetic, and later get values out in a given unit. For me, that makes the most intuitive sense.

However, I believe this comes down to syntax and maybe slightly on discoverability.
Feature-wise, this PR is identical to current.

If I understand right, we are looking at these before/after examples, so I think it is primarily a matter of syntax:

// Who knows what unit, but that is fine
var parsedMass = Mass.Parse(input);

// Some arithmetic maybe
var twiceMass = parsedMass * 2;

// Before/after
var grams1 = twiceMass.Grams;
var grams2 = twiceMass.AsGrams.Value;

// Before/after
var gramsString1 = twiceMass.As(MassUnit.Gram).ToString();
var gramsString2 = twiceMass.AsGrams.ToString();

@Muximize
Copy link
Contributor

You're right, it's mostly a syntax thing. But I don't think it is an improvement because it makes an (in my opinion, as explained above) unfortunate feature of UnitsNet more discoverable and easier to shoot yourself in the foot with. There should be some resistance and explicitness.

var parsedMass = Mass.Parse(input);

// Before/after
var someMass1 = parsedMass.ToUnit(MassUnit.Gram); // Quite explicitly changing the unit, good
var someMass2 = parsedMass.AsGrams;               // Would a new user understand what they are doing?

var someMass3 = parsedMass.As(MassUnit.Gram);     // Almost the same syntax, but returns a double?

@angularsen
Copy link
Owner

If binary size wasn't a concern, I would probably be open to having both options.
However, anything involving generating properties and methods for 1000+ units have significant impact on binary size.

I don't currently see enough benefit of changing to this new syntax style, and I agree with some of the drawbacks you outline @Muximize . It would cause a lot of breakage in people's code if v5 removed the old properties, and the benefit does not justify it for a minor syntax convenience.

I vote to close this PR.

@tmilnthorp
Copy link
Collaborator Author

Sorry guys has been a busy week, I haven't had time to check back in.

Fine by me, you won't offend :)

@angularsen
Copy link
Owner

I'm glad we can part as friends 😆

Just kidding, I appreciate your ideas 🙌

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.

3 participants