Fix inconsistent number style + replaced clientCulture with currentCulture (code *was* inconsistent)

[304NotModified]

fixes #2710

Created C# extensions for:

• DetermineClientLocale => HttpRequestExtensions
• Formatting number with client culture: NumberExtensions.ToNuGetNumberString

TODO:

• unit test UseClientCultureIfLanguageHeadersIsPresent fails because I use httpcontext.current. Ways to fix this:
  1. pass Request to the method everywhere instead of httpcontext.current,
  2. mock httpcontext.current.
  3. change unit test.
  4. remove the usage of HttpRequest in ToString (see
     Fix inconsistent number style + replaced clientCulture with currentCulture (code *was* inconsistent) #2716 (comment)) - I think 1 isn't good, because we're changing code solely for an unit test.
• alignment of downloads at package versions (should by right)

[dnfclas]

Hi @304NotModified, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

[dnfclas]

@304NotModified, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

[304NotModified]

Todo

• alignment of downloads at package versions (should by right)

[304NotModified]

I was thinking about this. I think the current solution isn't optimal. I should be:

request language => thread culture (UI + normal?) => toString methods with currentCulture.

This is more a .Net approach, and also, it fixes the tostrings of datetime etc. This also solves the unit test issue because we can set the current thread culture easily in an unit test.

It's a more refactor step than this one. So I fix it now, or in a later PR. What do you think?

[304NotModified]

Except the unit test, this is done.

[304NotModified]

Todo:

• search : package list/search is missing number style

[304NotModified]

Please give me feedback :)

besides unit test (where I need feedback for), everything is fixed.

[304NotModified]

Note: I have created & tested #2716 (comment) in a separate branch. (not in this PR, yet?)
This will fix the unit test and also I have successfully removed clientCulture everywhere - which is an improvement IMO. Let me know if I should merge it into this PR.

[maartenba]

"ToLocalizedNumberString()" sounds more clear

[304NotModified]

I also don't like the name ToNuGetNumberString. But it's consistent with DateTimeExtensions, like ToNuGetShortDateTimeString, ToNuGetShortDateString and ToNuGetLongDateString.

This is also mentioned in NumberExtensions:

[maartenba]

Added a minor comment to code (rename method so it's more clear what it does).

Regarding the unit test you can:

• safely mock the context
• I'd maybe pass the context into the extension methods at https://github.com/304NotModified/NuGetGallery/blob/304NotModified-2710-fix-inconsistent-number-style/src/NuGetGallery/Extensions/NumberExtensions.cs - it feels less clean when using the method but does make it perfectly clear that the context is needed. In fact I think passing in the Request object is enough - does not need the entire context

[304NotModified]

Thanks for the feedback and review.

I'd maybe pass the context into the extension methods at https://github.com/304NotModified/NuGetGallery/blob/304NotModified-2710-fix-inconsistent-number-style/src/NuGetGallery/Extensions/NumberExtensions.cs - it feels less clean when using the method but does make it perfectly clear that the context is needed. In fact I think passing in the Request object is enough - does not need the entire context

Passing the Request wasn't always possible. I could write some more overloads, but everything was getting ugly.

I only clean way was to implement my proposal of #2716 (comment)

Before executing an action (see AppController.OnActionExecuting), the CurrentCulture will be set by checking the Request.

I fully removed clientCulture. This is good because:

• DateTimeExtensions was using CurrentCulture already.
• The "official" way of .Net is using the CurrentCulture.
• Passing the ClientCulture was not nice.
• This makes it unit testable. The unit test didn't even had to change :)
• Parsing the Userlanguage is now done once per request.

Status: all done

[maartenba]

Thanks @304NotModified - will give this a thorough look tomorrow but looks sweet so far :)

[304NotModified]

OK, I see the unit test still fails at appveyor, but works locally. Have to check that first. I will let you know when it's fixed.

• fix unit test

edit: in an unit test "OnActionExecuting" was never called.

[304NotModified]

the unit test has been fixed. See build status / succeeding unit tests: https://ci.appveyor.com/project/304NotModified/nugetgallery/build/1.0.20

I needed a trick to execute the "OnActionExecuting" before calling the "action". This is (proper) implemented in the unit test class.

[maartenba]

Merged into dev instead - 440d22c

Thanks @304NotModified for your contribution!

[304NotModified]

No problem. But I preferred that I had created a new PR to "dev", because now the PR looks un-merged :(

[maartenba]

Ah no worries, that's GitHub I'm afraid.