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

Don't override .NET format string G #1206

Open
wants to merge 13 commits into
base: release/v6
Choose a base branch
from

Conversation

tmilnthorp
Copy link
Collaborator

@tmilnthorp tmilnthorp commented Feb 17, 2023

Fixes #1183

@tmilnthorp
Copy link
Collaborator Author

Fun times. Standard .NET formatting for double.MinValue is different between net48 and net7

@angularsen
Copy link
Owner

Awesome!

@tmilnthorp
Copy link
Collaborator Author

@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?

@tmilnthorp tmilnthorp marked this pull request as ready for review February 23, 2023 13:47
UnitsNet/GeneratedCode/Quantities/ElectricInductance.g.cs Outdated Show resolved Hide resolved
UnitsNet/GeneratedCode/Quantities/ElectricInductance.g.cs Outdated Show resolved Hide resolved
@@ -119,7 +119,7 @@ private static string FormatUntrimmed<TUnitType>(IQuantity<TUnitType> quantity,
formatProvider ??= CultureInfo.CurrentCulture;

if (string.IsNullOrWhiteSpace(format))
Copy link
Owner

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" />.
Copy link
Owner

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

UnitsNet/QuantityFormatter.cs Show resolved Hide resolved
@tmilnthorp tmilnthorp added this to the vNext milestone Feb 24, 2023
@tmilnthorp tmilnthorp changed the base branch from master to release/v6 February 24, 2023 12:41
@tmilnthorp
Copy link
Collaborator Author

@angularsen updated per your suggestions.

I think we should also remove

  • P/p
  • U/u
  • Q/q

These don't really add anything you can't do via the IQuantity interface.

Also maybe An/An should format the quantity using abbreviation n rather than just returning abbreviation n (e.g. 123.4 deg instead of the current deg).

@angularsen
Copy link
Owner

angularsen commented Feb 26, 2023

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.

@tmilnthorp
Copy link
Collaborator Author

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 15.3% m, or throw a FormatException? I'm leaning towards the exception.

@angularsen
Copy link
Owner

Regarding P, or any other default format string that's invalid for us. Should we just blindly allow strings of out say 15.3% m, or throw a FormatException? I'm leaning towards the exception.

FormatException 👍

Also maybe An/An should format the quantity using abbreviation n rather than just returning abbreviation n (e.g. 123.4 deg instead of the current deg).

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   

@stale
Copy link

stale bot commented Jun 18, 2023

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.

@stale stale bot added the wontfix label Jun 18, 2023
@angularsen angularsen added the pinned Issues that should not be auto-closed due to inactivity. label Jul 11, 2023
@stale stale bot removed the wontfix label Jul 11, 2023
@angularsen
Copy link
Owner

angularsen commented Jul 8, 2024

@lipchev It's been ages, but isn't this more or less ready to go? Still need to remove QUV? (already removed)

Can you update the PR description with a summary of the changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change pinned Issues that should not be auto-closed due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QuantityFormatter inconsequential format strings
2 participants