-
Notifications
You must be signed in to change notification settings - Fork 383
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
Don't override .NET format string G #1206
Don't override .NET format string G #1206
Conversation
Fun times. Standard .NET formatting for |
Awesome! |
@angularsen alright. I made everything match the runtime default (which is "G" by the way, not "g"). Obviously not an API breaking change, but it is a behavior change. Perhaps for the better. Merge now or is this a v6 thing? |
UnitsNet/QuantityFormatter.cs
Outdated
@@ -119,7 +119,7 @@ private static string FormatUntrimmed<TUnitType>(IQuantity<TUnitType> quantity, | |||
formatProvider ??= CultureInfo.CurrentCulture; | |||
|
|||
if (string.IsNullOrWhiteSpace(format)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is out of scope for this PR, but the IsNullOrWhitespace
seems non-standard.
The .NET number formatting source code seems to require exact matches on standard formats g
, g2
, x8
etc.
If there is no exact match on standard formats, such as trailing whitespace, it falls back to custom formatting, where they substitute placeholders like 0
and #
for numbers, or yyyy
for dates.
UnitsNet has defined some "standard" formats, but I'm wondering if we should split these up into standard formats and custom formats too. Standard formats ("g", "f") would require exact match, while custom formats ("q", "u") would allow some interpolation of placeholders.
CultureInfo.CurrentCulture = CultureInfo.GetCultureInfo("en");
var l = Length.FromFeet(1000.23456789);
void Print(string fmt) => l.ToString(fmt).Dump(fmt);
// ## Standard formats for UnitsNet (requires exact match)
Print("g"); // 1,000.23 ft (rounds to 2 decimals, .NET standard numeric format would render 1.23456789)
// Standard formats using the corresponding .NET standard numeric format for the double/decimal value
Print("c"); // ¤1,000.23 ft
Print("e"); // 1.000235e+003 ft
Print("f"); // 1000.235 ft
Print("n"); // 1,000.235 ft
Print("p"); // 100,023.457% ft
Print("r"); // 1000.23456789 ft
Print("#.0"); // 1000.2 ft
Print("00000.0"); // 01000.2 ft
// FormatException, only for integer types:
// Print("x");
// Print("d");
// ## Custom formats for UnitsNet (could allow interpolation)
Print("a"); // ft
Print("a1"); // '
Print("q"); // Length
Print("u"); // Foot
// Example of custom format interpolations that do not currently work:
Print("u (a)"); // ❌ "Foot", but expected "Foot (ft)"
Print("q,u"); // ❌ "Length", but expected "Length,Foot"
I'm not sure I see a big use case for custom formats in UnitsNet though, beyond sticking to .NET conventions.
Thoughts?
@@ -77,16 +84,11 @@ public static string Format<TUnitType>(IQuantity<TUnitType> quantity, string for | |||
/// <list type="bullet"> | |||
/// <item> | |||
/// <term>A standard numeric format string.</term> | |||
/// <description>Any of the standard numeric format for <see cref="IQuantity.Value" />, except for "G" or "g", which have a special implementation. | |||
/// "C" or "c", "E" or "e", "F" or "f", "N" or "n", "P" or "p", "R" or "r" are all accepted. | |||
/// <description>Any of the standard numeric format for <see cref="IQuantity.Value" />. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe mention that integral standard formats won't work, such as d/D and x/X.
Also link to the standard formats:
https://learn.microsoft.com/en-us/dotnet/standard/base-types/standard-numeric-format-strings#standard-format-specifiers
@angularsen updated per your suggestions. I think we should also remove
These don't really add anything you can't do via the Also maybe |
Yes I'm thinking the same for Q/U. Remove it. You don't obtain "Seconds" from TimeSpan.ToString() either. It's an odd thing to do and unit/quantity name is available elsewhere. Regarding P (percentage, numeric standard format), I think we should not support neither C (currency) nor P (percentage). They don't make sense. So I think we should choose the subset of standard formats we support, and document them. |
Sorry, I meant I'll remove Q/U/V. Regarding P, or any other default format string that's invalid for us. Should we just blindly allow strings of out say |
FormatException 👍
It would be more consistent, but then you can't control the number formatting, for example to 2 digits after radix. I think abbreviations are more useful as a custom format for string interpolation: var x = $"{myLength.Value:f2} {myLength:a0}"; // 5 ft
var y = $"{myLength.Value:f2} {myLength:a1}"; // 5 ' I think this is how it would look like: // Standard formats with .NET standard/custom numeric format + default unit abbreviation
Print("e"); // 1.000235e+003 ft
Print("e2"); // 3.14e+000 ft
Print("f"); // 1000.235 ft
Print("f1"); // 1000.2 ft
Print("n"); // 1,000.235 ft
Print("n2"); // 1,000.23 ft
Print("r"); // 1000.23456789 ft
Print("#.0"); // 1000.2 ft
Print("00000.0"); // 01000.2 ft
// Custom formats -- For string interpolation.
Print("a"); // ft
Print("a1"); // ' // FormatException thrown by dotnet, only for integer types:
// Print("x");
// Print("d");
// FormatException thrown by us, no longer supported
Print("c"); // ¤1,000.23 ft
Print("p"); // 100,023.457% ft
Print("q"); // Length
Print("u"); // Foot
Print("v"); // 1000.23456789 |
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. |
@lipchev It's been ages, but isn't this more or less ready to go? Can you update the PR description with a summary of the changes? |
@angularsen do we still want this? can I include it in my v6 proposal? |
Yeah I belive this is still useful. What do you mean by including it in your v6 proposal? |
Don't worry, I'll split it and it's still going to be...godzilla 🐲 PS Lol, Is that a dragon emoji? I don't think you should be allowed to go around, claiming you're old.. 🤣 |
Lol 😆 |
- [x] `QuantityFormatter` defaults to the "G" format for null or empty string - [x] `QuantityFormatter` no longer supports the `U`/`V`/`Q` formats (an exception is thrown) - [x] `QuantityFormatter` explicitly throws for the `Cx`/`Px` formats - [x] `QuantityFormatter` shouldn't throw for something like "P1: #.00" (instead it should output something like "P1: 12.34 mg") - [x] `QuantityFormatter` unless explicitly given an incorrect `Ax` specifier, the `QuantityFormatter` uses the "default unit abbreviation" - which maybe `string.Empty` (if none are defined for a given unit) Fixes #1183 Duplicate of #1206 CC @tmilnthorp --------- Co-authored-by: Andreas Gullberg Larsen <andreas.larsen84@gmail.com>
Merged in #1450 |
Fixes #1183