Handle download count round up to 1E+3K for when 999,500 <= count <=999,999 #4255

erdembayar · 2021-09-09T15:57:54Z

Bug

Fixes: NuGet/Home#8800
Fixes: NuGet/Home#10864

Regression? Last working version:

Description

For number higher than 999.5 fractional number(it's less than 1000) it round up to 1000 but it ends up 1E+3K, 1E+3M, 1E+3G text. This problem can be solved in many different ways, but I selected current approach which introduce least breaking change on UI screen, my change affect 0.1% ~ 1/1000 cases, so 999/1000 cases users wouldn't notice it any difference.

Update

Added localization after PR review comment. Now english users see 1B instead of 1G, but depending on localization other users see differently.

Previous experience on PMUI:

New experience on PMUI:

Original number - Previous experience - New experience

PR Checklist

erdembayar · 2021-09-09T15:58:15Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Utility/UIUtility.cs

                ++exp;
            }

+            string format = v < 999 ? "{0:G3}{1}" : "{0:G4}{1}"; // If number is 999561 then v = 999.56 so with G3 which result in 1000k ~ 1E+03K, but with G4 it result in 999.6K.


I added logic for handling v >= 999, we handle with 1 more precision , but I didn't for v< 999 otherwise it's kind of breaking experience for many users seeing too much detailed information.

Isn't showing a 4th digit of precision when all other cases only show 3 also a kind of "breaking" experience?

How about if (v > 999.0) { v = 999.0; }?

@zivkan
Ok. Taken your suggestion. Please check now.

erdembayar · 2021-09-09T16:00:00Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Utility/UIUtility.cs

@@ -64,15 +64,17 @@ public static string NumberToString(long number, IFormatProvider culture)

            while (v >= 1000)
            {
-                v /= 1000;
+                v = Math.Round(v / 1000, 2);


On PMUI we don't see 3 digit precision, most 2 digits like below, otherwise previous logic having too detailed info causes problem.

test/NuGet.Clients.Tests/NuGet.PackageManagement.UI.Test/ConverterTests.cs

donnie-msft · 2021-09-10T00:17:04Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Utility/UIUtility.cs

@@ -47,8 +47,8 @@ private static bool IsHttpUrl(Uri uri)
            return (uri.Scheme == Uri.UriSchemeHttp || uri.Scheme == Uri.UriSchemeHttps);
        }

-        private static readonly string[] _scalingFactor = new string[] {
-            String.Empty,
+        private static readonly string[] ScalingFactor = new string[] {


Shouldn't this stay as _scalingFactor?
https://github.com/NuGet/NuGet.Client/blob/dev/docs/coding-guidelines.md#c-coding-style

This is IDE suggested fix, it looks rule is bit different for static readonly variable.

NuGet.Client/src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/IDE/VSSolutionManager.cs

Line 42 in f4ae349

private static readonly INuGetProjectContext EmptyNuGetProjectContext = new EmptyNuGetProjectContext();

I think this whole algorithm is too complex and there's a bug here where billions will look like 1.23G because G is specified for billions instead of B. Does this scaling factor need to know about byte size and item count? If so then it would need to differentiate them.

I think we should simplify this method and get rid of the loop which is confusing and made you add this silly looking if statement:

// If number is 999561 then v = 999.56 so with G3 which result in 1000k ~ 1E+03K. if (v > 999) { v = 999; }

What do you think about this instead?

public static string NumberToString(long number, IFormatProvider culture) { const double thousand = 1000D; const double million = 1000000D; const double billion = 1000000000D; const double trillion = 1000000000000D; if (number >= trillion) { return (number / trillion).ToString("0.##T", culture); } if (number >= billion) { return (number / billion).ToString("0.##B", culture); } if (number >= million) { return (number / million).ToString("0.##M", culture); } if (number >= thousand) { return (number / thousand).ToString("0.##K", culture); } return number.ToString("G0", culture); }

Then you can get rid of the ScalingFactor array as well and this method makes waaaaay more sense.

We might need to methods:
ByteCountToString(): Returns the number as bytes so KB, MB, GB, TB (kilobytes, megabytes, gigabytes, terabytes)
CountToString(): Returns a number as a count so K, M, B, T (thousand, million, billion, trillion)

because G is specified for billions instead of B.

G is the SI (standard international) prefix. If we want to use B in English, then we need to make it a string resource to allow the translators to localize this into the correct value for each language. So that part of it is by-design, I think. See also NuGet/Home#10864

Well yeah while we're here we might as well localize it! Fix two bugs with one stone

@jeffkl @zivkan
I took Jeff's code as base and added localization text, please check now.
Number - Previous experience - New experience

jeffkl

Feel free to ping me to discuss if you want, I really think we should fix this code as a whole

jeffkl · 2021-09-10T17:03:04Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Utility/UIUtility.cs

@@ -47,8 +47,8 @@ private static bool IsHttpUrl(Uri uri)
            return (uri.Scheme == Uri.UriSchemeHttp || uri.Scheme == Uri.UriSchemeHttps);
        }

-        private static readonly string[] _scalingFactor = new string[] {
-            String.Empty,
+        private static readonly string[] ScalingFactor = new string[] {


I think this whole algorithm is too complex and there's a bug here where billions will look like 1.23G because G is specified for billions instead of B. Does this scaling factor need to know about byte size and item count? If so then it would need to differentiate them.

I think we should simplify this method and get rid of the loop which is confusing and made you add this silly looking if statement:

// If number is 999561 then v = 999.56 so with G3 which result in 1000k ~ 1E+03K. if (v > 999) { v = 999; }

What do you think about this instead?

public static string NumberToString(long number, IFormatProvider culture) { const double thousand = 1000D; const double million = 1000000D; const double billion = 1000000000D; const double trillion = 1000000000000D; if (number >= trillion) { return (number / trillion).ToString("0.##T", culture); } if (number >= billion) { return (number / billion).ToString("0.##B", culture); } if (number >= million) { return (number / million).ToString("0.##M", culture); } if (number >= thousand) { return (number / thousand).ToString("0.##K", culture); } return number.ToString("G0", culture); }

Then you can get rid of the ScalingFactor array as well and this method makes waaaaay more sense.

We might need to methods:
ByteCountToString(): Returns the number as bytes so KB, MB, GB, TB (kilobytes, megabytes, gigabytes, terabytes)
CountToString(): Returns a number as a count so K, M, B, T (thousand, million, billion, trillion)

aortiz-msft

Approved for insertion into dev branch.

erdembayar · 2021-09-10T18:37:01Z

@jeffkl
Yes, I thought same approach as yours. But I didn't want to make breaking change this time, let's keep it as it's now.
With 0.##K format it introduces extra precision compare to previous experience, we better to consult PM before making any breaking change.

We previously had discussion on replacing G with B then decided not to act that time. For people on other side of world current one is correct.
Also as far I can find only used with download count, it's not used with size conversion with bytes, so not concern for now.

jeffkl · 2021-09-10T18:49:42Z

I don't think changing the precision would be a breaking change but we could keep it as-is with 0.#K right? We can localize the G/B in a subsequent commit but I think we need to simplify this method, especially if the unit test coverage is great which it looks like is based on your added test case data.

jeffkl

Looks good to me, thanks for localizing it too. Update the PR description to say it fixes NuGet/Home#10864 ?

zivkan · 2021-09-11T13:14:00Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Utility/UIUtility.cs

+            {
+                return string.Format(
+                   culture,
+                   "{0:G3}{1}",


Wouldn't it be better to include the {0:G3} in the localized string? Maybe some cultures prefer to have a space between the number and the suffix, while others don't. For example 123 M vs 123M

@zivkan
I moved into localized string, please review again.

zivkan · 2021-09-11T13:16:17Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Resources.resx

+  </data>
+  <data name="Thousand" xml:space="preserve">
+    <value>K</value>
+    <comment>1.000 in short format notation</comment>


English uses , for thousands separators, so while all of these comments use . which is probably more common across the world, given our code and comments are in English, it looks a bit odd, and this one (thousands) can be mistaken for the number one (precise to 3 decimal places).

dominoFire

Some comments on strings. I just want to make sure loc team will not get confused with two different meanings in the same string resource

src/NuGet.Clients/NuGet.PackageManagement.UI/Resources.resx

dominoFire · 2021-09-13T19:27:52Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Resources.resx

+  </data>
+  <data name="Trillion" xml:space="preserve">
+    <value>{0:G3}T</value>
+    <comment>Trillions in short format notation</comment>


Please specify which trillions are you referring to:

Suggested change

<comment>Trillions in short format notation</comment>

<comment>American trillions in short format notation</comment>

@dominoFire
Does EU and US trillions use different notations?

https://en.wikipedia.org/wiki/Long_and_short_scale

https://en.wikipedia.org/wiki/Long_and_short_scale

Thanks you. Now I understand what milliard actually means, previously I always thought it's just another regional spelling for million.

aortiz-msft · 2021-09-14T00:53:34Z

test/NuGet.Clients.Tests/NuGet.PackageManagement.UI.Test/ConverterTests.cs

+        [InlineData(1939, "1.94K", "en-US")]
+        [InlineData(1939, "1,94K", "da-DK")]
+        [InlineData(9456, "9.46K", "en-US")]
+        [InlineData(9456, "9,46K", "da-DK")]


[InlineData(9456, "9,46K", "da-DK")]

Could we also add tests for B vs G, since Andy pointed out that G will be used for locales following the metric system?

@aortiz-msft
I thought about it but I believe it's available after localization team make translation for each different cultures. @dominoFire Please correct me if I'm wrong.

Also, since NuGet.Client's localization happens in a different repo, we can't have unit tests that work the same and still pass both locally (that doesn't run MicroBuild's localization task) and on CI (which does run MicroBuild's localization task).

I believe that if we migrated to the same localization system/format that the rest of the dotnet org uses, then local builds would include it.

erdembayar added 3 commits September 8, 2021 18:44

Fix formatting includes E exponential.

f1aeeef

Remove peta part.

54d9b20

Add tests.

c576035

erdembayar requested a review from a team as a code owner September 9, 2021 15:57

erdembayar commented Sep 9, 2021

View reviewed changes

Address PR comment.

b43a96b

zivkan approved these changes Sep 9, 2021

View reviewed changes

test/NuGet.Clients.Tests/NuGet.PackageManagement.UI.Test/ConverterTests.cs Show resolved Hide resolved

test/NuGet.Clients.Tests/NuGet.PackageManagement.UI.Test/ConverterTests.cs Outdated Show resolved Hide resolved

Remove duplicate tests.

248eb20

donnie-msft approved these changes Sep 10, 2021

View reviewed changes

jeffkl requested changes Sep 10, 2021

View reviewed changes

aortiz-msft self-requested a review September 10, 2021 17:43

aortiz-msft approved these changes Sep 10, 2021

View reviewed changes

erdembayar mentioned this pull request Sep 10, 2021

NuGet packages downloaded over 1 billion times show the wrong unit NuGet/Home#10864

Closed

erdembayar requested a review from jeffkl September 10, 2021 18:37

erdembayar force-pushed the dev-eryondon-8800 branch from 5eca391 to 81160a2 Compare September 11, 2021 00:52

Address latest comment and add localization text.

a456837

erdembayar force-pushed the dev-eryondon-8800 branch from 81160a2 to a456837 Compare September 11, 2021 00:58

jeffkl approved these changes Sep 11, 2021

View reviewed changes

zivkan reviewed Sep 11, 2021

View reviewed changes

erdembayar added 2 commits September 11, 2021 08:09

Address comment about number seperator.

b1e289a

Add localization of format.

8695bb6

erdembayar force-pushed the dev-eryondon-8800 branch from 9fcdc5d to 8695bb6 Compare September 11, 2021 16:38

dominoFire reviewed Sep 13, 2021

View reviewed changes

Address PR comment by Fernando.

972b348

erdembayar merged commit 0d46dd8 into dev Sep 14, 2021

erdembayar deleted the dev-eryondon-8800 branch September 14, 2021 00:37

aortiz-msft reviewed Sep 14, 2021

View reviewed changes

stamminator mentioned this pull request Jan 27, 2022

Download counts do not appear for packages suggested by Package Recommender NuGet/Home#10714

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle download count round up to 1E+3K for when 999,500 <= count <=999,999 #4255

Handle download count round up to 1E+3K for when 999,500 <= count <=999,999 #4255

erdembayar commented Sep 9, 2021 •

edited

Loading

erdembayar Sep 9, 2021

zivkan Sep 9, 2021

erdembayar Sep 9, 2021

erdembayar Sep 9, 2021

donnie-msft Sep 10, 2021

erdembayar Sep 10, 2021 •

edited

Loading

jeffkl Sep 10, 2021

zivkan Sep 10, 2021

jeffkl Sep 10, 2021

erdembayar Sep 11, 2021

jeffkl left a comment

jeffkl Sep 10, 2021

aortiz-msft left a comment

erdembayar commented Sep 10, 2021

jeffkl commented Sep 10, 2021

jeffkl left a comment

zivkan Sep 11, 2021

erdembayar Sep 11, 2021

zivkan Sep 11, 2021 •

edited

Loading

dominoFire left a comment

dominoFire Sep 13, 2021

erdembayar Sep 13, 2021 •

edited

Loading

zivkan Sep 13, 2021

erdembayar Sep 13, 2021

aortiz-msft Sep 14, 2021

erdembayar Sep 14, 2021

zivkan Sep 14, 2021

	<comment>Trillions in short format notation</comment>
	<comment>American trillions in short format notation</comment>

Handle download count round up to 1E+3K for when 999,500 <= count <=999,999 #4255

Handle download count round up to 1E+3K for when 999,500 <= count <=999,999 #4255

Conversation

erdembayar commented Sep 9, 2021 • edited Loading

Bug

Description

Update

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffkl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aortiz-msft left a comment

Choose a reason for hiding this comment

erdembayar commented Sep 10, 2021

jeffkl commented Sep 10, 2021

jeffkl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivkan Sep 11, 2021 • edited Loading

Choose a reason for hiding this comment

dominoFire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar commented Sep 9, 2021 •

edited

Loading

erdembayar Sep 10, 2021 •

edited

Loading

zivkan Sep 11, 2021 •

edited

Loading

erdembayar Sep 13, 2021 •

edited

Loading