Skip to content
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

Remove conversion overloads from ToString implementations #545

Closed
tmilnthorp opened this issue Nov 1, 2018 · 7 comments
Closed

Remove conversion overloads from ToString implementations #545

tmilnthorp opened this issue Nov 1, 2018 · 7 comments

Comments

@tmilnthorp
Copy link
Collaborator

I would like to split up the code responsible for unit conversion and ToString.

Since we have the ToUnit method, we could simplify the ToString implementations and implement IFormattable.

This would leave the following ToString overloads:

public override string ToString();
public string ToString(string format);
public string ToString(IFormatProvider provider);
public string ToString(string format, IFormatProvider provider);

For example to get a length string in inches, we currently do the following:

var length = Length.FromMeters( 3.0 );
var lengthInInchesString = length.ToString( LengthUnit.Inch );

Today, this will achieve the same thing

var length = Length.FromMeters( 3.0 );
var lengthInInchesString = length.ToUnit( LengthUnit.Inch ).ToString();

We could even change the conversion properties to return a converted quantity rather than a double (public Length Inches in this example) and do the following:

var length = Length.FromMeters( 3.0 );
var lengthInInchesString = length.Inches.ToString();
@angularsen
Copy link
Owner

My initial thought is, I like it. It feels more consistent that ToString() only uses whatever value and unit it was created with and if you want something else you convert it first. It also resembles how DateTimeOffset.UtcNow.ToString() is different than DateTimeOffset.Now.ToString(), if you consider different timezones as a kind of unit conversion, and you can't convert to UTC using only format string - you need to call .ToUniversalTime() or .ToLocalTime() first.

However, it does might make it less discoverable (not obvious you need to call ToUnit() first, but we could mention it in ToString xmldoc). Also, I assume we will break a lot of consumer code since ToString()'ing into a specific unit is a pretty common usecase.

IFormattable format string

IFormattable takes a format string. I skimmed through the docs, it even has a Temperature example. Here they demo how you can pass "F" to get Fahrenheit or "C" for Celsius, but the number formatting is hard coded.

Maybe we could do something clever to allow you to control both unit and number formatting in the format string? Or maybe it's just a deathtrap waiting to cause bugs for consumers, who will never memorize the magic strings. I was thinking something like this:

var meter = Length.FromMeters(1);
meter.ToString("Foot_F2"); // "3.28 ft"
meter.ToString("Foot_F3"); // "3.281 ft"
meter.ToString("Foot_F4"); // "3.2808 ft"
meter.ToString("ft_F4"); // "3.2808 ft" may throw if ambiguous abbreviation
Console.WriteLine("{0:ft_F4}", meter); // "3.2808 ft" may throw if ambiguous abbreviation

F2 etc is a custom number formatting for floating/fixed point number types.

What do you think of something like this? Too complicated or error prone?

@tmilnthorp
Copy link
Collaborator Author

I think adding the magic strings like that example of ToString makes it a kind of a "god method" that does multiple things. ToString should just worry about formatting the current object's representation to a string (single responsibility).

The ToUnit method is very explicit, strongly typed, and IMHO intuitive. I am not sure what to do if there is ambiguity by string. Also how do you shorten things like W/m^2? Using ToUnit avoids all these issues.

It will indeed break consumer code, but at least it's a easy fix. Perhaps this would be a good idea for v4 in this case, unless you'd rather make a v5 more immediate?

@tmilnthorp
Copy link
Collaborator Author

tmilnthorp commented Nov 2, 2018

I think I would much rather see something like: "V:E3 [U]" to make 1054.32179 meters display as "1.054E+003 [m]" where V = value and U = unit abbreviation. This is kind of like DateTime where you specify HH, etc.

@angularsen
Copy link
Owner

Feature freeze was end of October, so we are stretching it, but if you can get the PR in quick I see no problem in removing the overloads as you initially described.

However, IFormattable and figuring out how we want format strings to work will take more time and is probably best to defer to v5.

We already have this in v3: Length.FromMeters(1).ToUnit(LengthUnit.Centimeter).ToString() // "100 cm"

@tmilnthorp
Copy link
Collaborator Author

So for IFormattable we should make a list:

  • "G" = current default implementation
  • "A1", "A2"... "An" = abbreviation 1 or 2 or n in the list
  • "A" = first abbreviation (equal to "A1")
  • "Q" = quantity name (e.g. "Length")
  • "V" value using default ToString() for double/decimal/etc.

For more complicated strings I think you'd want to specify on a more granular level. We don't want to parse all the number format types. For formatting the value as double, etc. you might want to do the following:

var length = Length.FromMeters( 3.0 );
string.Format( "{0,10:E3} [{1:A1}]", length.Value, length );

@angularsen
Copy link
Owner

Sounds reasonable, please move this to a separate issue to gather all ideas on IFormattable and format strings in one place.

@angularsen
Copy link
Owner

Closing this issue as it will be addressed in v4.

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

No branches or pull requests

2 participants