Skip to content

Initial implementation of IFormattable #599

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 26 commits into from
Mar 1, 2019

Conversation

tmilnthorp
Copy link
Collaborator

@tmilnthorp tmilnthorp commented Feb 4, 2019

Fixes #579

Initial go at #579. Didn't get much input there, so see if we like it!

Exception should show provided format string, not internally modified one
@tmilnthorp
Copy link
Collaborator Author

While working on this I realized... any reason we use GlobalConfiguration.DefaultCulture for everything? Why not just use CultureInfo.CurrentCulture? I think that is more what a user would expect. If you changed CurrentCulture right now, you'd also have to change the GlobalConfiguration.DefaultCulture for it to be picked up.

@angularsen
Copy link
Owner

Hm, good point. I don't really see a good reason to keep it, I don't see what it gives us.

To add to it, there is the distinction of CultureInfo.CurrentCulture and CultureInfo.CurrentUICulture and I think we only have a configuration for the latter. If I recall correctly, the guidelines say something along the lines of

  • CurrentUICulture for translations/localizations
  • CurrentCulture for number formatting

It's probably more nuanced than that, but that's how I remember it. I think you have to manipulate CultureInfo.CurrentCulture to change the default number formatting used by ToString(). If that is correct, then what you propose would be more consistent too. It's a breaking change though, so something to add to #563 .

@angularsen
Copy link
Owner

I'll review the PR later.

@tmilnthorp
Copy link
Collaborator Author

I re-merged with after the script split. I'm curious your thoughts!

Adding test to make sure format strings are case insensitive
Add test for exception with invalid format string
Check null or empty only for G format string. Change ToUpper to ToUpperInvariant for correct behavior with some cultures.
Adding U format string for unit
String can only be null/empty now
@tmilnthorp
Copy link
Collaborator Author

Looking at the standard format strings for:

It seems that they use case insensitivity for the default format strings.

Custom format strings tends to be case sensitive though. Should we follow a similar paradigm? What denotes a custom format vs standard though is what we should probably figure out! 😃

var abbreviations = UnitAbbreviationsCache.Default.GetUnitAbbreviations<AccelerationUnit>(Unit, formatProvider);
return abbreviations[digits];
case "V":
return Value.ToString(formatProvider);
Copy link
Owner

@angularsen angularsen Feb 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe support significant digits after radix point here? Mass.FromKilograms(15.3).ToString("V4") could output 15.3000 similar to 15.3.ToString(F4) would.

Copy link
Owner

@angularsen angularsen Feb 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the same spirit V#### could maybe output 15.3, similar to how 15.3.ToString("0.####") would. # is used to represent up to N numbers, but omit any trailing zeros when used after radix point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what S does (must be given a number). You want both to do the same thing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always wonder at what point do we stop with the strings and let the user just do a string.format("0.####", quantity.Value) ❓

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair points, I've had the same thought.
Doesn't S also include the abbreviation? Maybe I read it wrong.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Length.FromCentimeters(15.34000).ToString(null, 5) => 15.34 cm, so yes, it does the # thing.

So my proposal would be:
Length.FromCentimeters(15.34000).ToString("V####") to get 15.34 without the unit abbreviation.

It's valid on its own, but I agree with you, at some point people should just do standard/custom numeric formatting of the Value property. I can agree to don't do the V#### usecase on that ground.

@angularsen
Copy link
Owner

angularsen commented Feb 25, 2019

It seems that they use case insensitivity for the default format strings.

Actually, it seems to me they are in fact different.

"g" short time vs "G" long time.

Same for "d"/"D", "f"/"F", "t"/"T", "u"/"U" differing on short/long or short/full formats.
These are all standard formats and that to me adds weight to the argument to keep it all case sensitive - to avoid any confusion. For instance "o" and "O" are identical in Date, which to me just adds confusion when most other standard formats have different outputs for lower and upper case.

What denotes a custom format vs standard though is what we should probably figure out! 😃

From Date docs:

Any date and time format string that contains more than one character, including white space, is interpreted as a custom date and time format string; for more information, see Custom Date and Time Format Strings.

The numeric docs say pretty much the same thing, except the also accept an optional number xx behind the letter.

The way I read it, "standard" here only means their vetted, recommended, typical formats. The formats they want the .NET ecosystem to use. While custom is anything else, with a combination of date and time parts as well as free text fashioned together in any way you like.

In our case, I'm thinking standard format specifiers could maybe be like this:

  • g: 15.1 kg
  • s4: 15.1000 kg
  • S4: 15.1 kg (using # to omit trailing zeros)

Custom format specifiers:

  • a: kg
  • a2: (2nd abbreviation for kilogram, or empty string(?) if not exists)
  • q: Mass
  • u: Kilogram
  • v: 15.1
  • v4: 15.1000
  • v####: 15.1

Then later, if we add human readable strings, we could add

  • Q: Bit Rate
  • U: Bit per Second
  • P or Up: Bits per Second

Update: On second thought, maybe human readable strings should be the lowercase format specifiers to hint at them being the preferred format specifiers, while enum names for quantity and unit as uppercase specifiers to hint at them being the secondary choice.

Pull IFormattable code into  QuantityFormatter
Simplify
Rename variable
@tmilnthorp
Copy link
Collaborator Author

You're right... I think I'm going crazy 🤡 I've had a sick baby at home and not much sleep 😆

I'll make them lower-case for now.

Making format strings case sensitive and making all lower-case for now
@angularsen
Copy link
Owner

I can relate :-D a solid case of the chickenpox the past two days here reaching its peak just about now :-)

@angularsen
Copy link
Owner

I think with your latest changes, this is getting ready to merge. We don't need to get this perfect on the first try, let's just try something and see how it feels. We can leave v4 and v#### out for now.

Adding doc
Adding doc for valid format strings
@tmilnthorp
Copy link
Collaborator Author

Oh man. Hope everything's better!

I've added doc about the valid format strings.

Also, I think we should obsolete the following methods as part of this PR:

  • public string ToString([CanBeNull] IFormatProvider provider)
    • Use ToString("g", provider)
  • public string ToString([CanBeNull] IFormatProvider provider, int significantDigitsAfterRadix)
    • Use ToString("s2", provider) (or however many digits in format string)
  • public string ToString([CanBeNull] IFormatProvider provider, [NotNull] string format, [NotNull] params object[] args)
    • Use string.format using value, unit, etc.

Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice! Some feedback.

I also, I think we should obsolete the following methods as part of this PR:

Sounds good to me.

Obsolete ToString methods. Add inheritdoc. Update tests for obsolete methods.
Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More feedback :-) This should be getting ready to merge.

Update doc. Obsolete IQuantity ToString overloads and replace code for obsolete calls. Throw FormatException when invalid abbreviation index is requested and add test.
Converting test to use MemberData rather than rely on strings for checking values
Removing duplicate test and making s format string test data-driven
Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a little bit more :-)

@angularsen
Copy link
Owner

angularsen commented Feb 28, 2019

You also have a bit of merge conflicts to deal with ;-) Fortunately almost all of them are just regens.

@tmilnthorp
Copy link
Collaborator Author

Merged and updated. I think it's done! 🎉

Update comments
Update comments
@angularsen angularsen merged commit d819b99 into angularsen:master Mar 1, 2019
@angularsen
Copy link
Owner

Wohoo :) Awesome job on this!

@MrFoged
Copy link

MrFoged commented Mar 12, 2019

Could you give a little insight into how to use the new string.Format?

Right now i have this code:
.ToString(new CultureInfo("en-US"), "{0:N1} {1}") which it tells me to change but to what? :-)

@tmilnthorp
Copy link
Collaborator Author

tmilnthorp commented Mar 12, 2019

The equivalent of that should be:

string.Format(new CultureInfo("en-US"), "{0:N1} {1:a}", quantity.Value, quantity);

@angularsen
Copy link
Owner

Updated docs in String Formatting · angularsen/UnitsNet Wiki

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.

Enhanced IFormattable/ToString Support
3 participants