Formats file sizes using the user's locale by caugner · Pull Request #11893 · nextcloud/server

caugner · 2018-10-17T19:45:11Z

Fixes#7837

caugner · 2018-10-17T19:47:56Z

Before:

After:

rullzer

Awesome.
And with tests :) ❤️

rullzer · 2018-10-17T20:24:34Z

First-time contributor

Welcome to the club @caugner

caugner · 2018-10-17T21:48:55Z

Unfortunately the new test fails (I tried to run them locally, but phantom failed to spawn, I'm using NixOS, which doesn't support running the downloaded binaries, and my locally installed phantom v2 binary doesn't seem to be taken into consideration; also I didn't find a quick way to run the tests in Firefox instead):

I have two possible explanations:

Either my sinon.stub(OC, 'getLocale') approach doesn't work.
Or phantomjs doesn't support toLocaleString (at least not with the specified locale de).

kesselb · 2018-10-18T03:56:13Z

2. Or `phantomjs` doesn't support `toLocaleString` (at least not with the specified locale `de`).

ariya/phantomjs#12581 (comment) 😞

ChristophWurst · 2018-10-18T06:31:32Z

First-time contributor

Awesome first-time contribution ✌️ Thanks a lot 🚀

MorrisJobke

Nice 👍 but it does not work with something like de_DE:

parseFloat('8.5').toLocaleString('de_DE');
RangeError: invalid languagetag: de_DE

I still have no idea how to work around this phantomJS limitation :/ Maybe @danxuliu or @skjnldsv can help here

caugner · 2018-10-18T10:01:33Z

@MorrisJobke Yes, looks like we need to convert the Nextcloud locale we get from OC.getLocale() into a BCP-47 compliant language tag (which uses a dash instead of an underscore to separate language code from region code): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl#locales_argument

caugner · 2018-10-18T20:26:30Z

@MorrisJobke

Nice +1 but it does not work with something like de_DE:

I conducted a short test to see if it suffices to locale.replace(/_/g, "-") (see 3d438c3) in order to get a BCP-47 compliant language tag and indeed it does (so I will commit it in a minute):

Note that localizing numbers not only affects the decimal separator, but also the digits itself, e.g.:

١٢٣٫٤٥ ar
১২৩.৪৫ as
১২৩.৪৫ bn
123,45 de
123.45 en
۱۲۳٫۴۵ fa
१२३.४५ mr
၁၂၃.၄၅ my
१२३.४५ ne

(See https://pastebin.com/URn57qJG for the full test output.)

caugner · 2018-10-18T20:26:36Z

@MorrisJobke @danxuliu @skjnldsv Regarding the phantomjs incompatibility, I can think of two workarounds:

Test that the output is equal to the output of #toLocaleString.
Skip the test if the output of 123.45.toLocaleString("de") does not equal 123,45 (see 71eb19b).

rullzer · 2018-10-19T08:45:35Z

I would be fine to get this in for now. If phantomjs doesn't play nice then so be it.

caugner

Need to cover case where getLocale() is undefined.

caugner

Took stub fallback value from wrong method.

skjnldsv · 2018-10-19T10:24:12Z

@MorrisJobke
Nice +1 but it does not work with something like de_DE:
I conducted a short test to see if it suffices to locale.replace(/_/g, "-") (see 3d438c3) in order to get a BCP-47 compliant language tag and indeed it does (so I will commit it in a minute):
Note that localizing numbers not only affects the decimal separator, but also the digits itself, e.g.:

@caugner
I implemented a feature recently that get the closest string locale code from the locale.
See #11134: findLanguageFromLocale

caugner · 2018-10-19T12:21:56Z

@skjnldsv Afaik we don't need just the primary language here (which findLanguageFromLocale derives from the locale), but rather the locale (but BCP-47 compliant).

Even though "[BCP-47 compliant] language tag" may sound like it only contains the language, it may include e.g. the region. In fact, this is relevant, as number formatting does differ between regions with the same language:

1234.56.toLocaleString('de-DE')// "1.234,56"1234.56.toLocaleString('de-CH')// "1’234.56"

rullzer · 2018-10-24T07:10:58Z

I took the liberty of rebasing

ChristophWurst

Nice enhancement. Code looks good and is covered by tests -> 👍

humanFileSize: add test with locale humanFileSize: use canonical locale humanFileSize: skip test w/o toLocaleString support humanFileSize: stub getCanonicalLocale OC.getCanonicalLocale: cover undefined case humanFileSize: fix getCanonicalLocale stub Signed-off-by: Claas Augner <git@caugner.de>

caugner · 2018-10-24T09:11:24Z

+ });
+
+ it("Returns primary locales as is", function() {
+ expect(OC.getCanonicalLocale('de')).toEqual('de');


Rather than calling getCanonicalLocale with the locale as a parameter, set the localeStub to return this
Suggested change
expect(OC.getCanonicalLocale('de')).toEqual('de');
localeStub.returns('de');
expect(OC.getCanonicalLocale()).toEqual('de');

caugner · 2018-10-24T09:11:33Z

+ expect(OC.getCanonicalLocale('zu')).toEqual('zu');
+ });
+ it("Returns extended locales with hyphens", function() {
+ expect(OC.getCanonicalLocale('az_Cyrl_AZ')).toEqual('az-Cyrl-AZ');


Suggested change
expect(OC.getCanonicalLocale('az_Cyrl_AZ')).toEqual('az-Cyrl-AZ');
localeStub.returns('az_Cyrl_AZ');
expect(OC.getCanonicalLocale()).toEqual('az-Cyrl-AZ');

caugner

Need to fix test for getCanonicalLocale

caugner · 2018-10-24T09:14:03Z

+
+ it("Returns primary locales as is", function() {
+ expect(OC.getCanonicalLocale('de')).toEqual('de');
+ expect(OC.getCanonicalLocale('zu')).toEqual('zu');


Suggested change
expect(OC.getCanonicalLocale('zu')).toEqual('zu');
localeStub.returns('zu');
expect(OC.getCanonicalLocale()).toEqual('zu');

caugner · 2018-10-24T09:14:58Z

+ });
+ it("Returns extended locales with hyphens", function() {
+ expect(OC.getCanonicalLocale('az_Cyrl_AZ')).toEqual('az-Cyrl-AZ');
+ expect(OC.getCanonicalLocale('de_DE')).toEqual('de-DE');


Suggested change
expect(OC.getCanonicalLocale('de_DE')).toEqual('de-DE');
localeStub.returns('de_DE');
expect(OC.getCanonicalLocale()).toEqual('de-DE');

Signed-off-by: Claas Augner <git@caugner.de>

MorrisJobke · 2018-10-24T12:45:31Z

There is still an error in the JSUnit tests :'(

caugner · 2018-10-25T08:46:46Z

@MorrisJobke Looks like either a test artefact or an unrelated test failing: https://drone.nextcloud.com/nextcloud/server/11798/284

PhantomJS 2.1.1 (Linux 0.0.0) ERROR
{
"message": "An error was thrown in afterAll\nExpected false to equal true.\napps/files_versions/tests/js/versionmodelSpec.js:76:47\ncore/vendor/jquery/dist/jquery.min.js:2:56437\nj@core/vendor/jquery/dist/jquery.min.js:2:27194\nfireWith@core/vendor/jquery/dist/jquery.min.js:2:54761\ncore/vendor/jquery/dist/jquery.min.js:2:56856\ncore/js/files/client.js:9:30735\nlib$es6$promise$$internal$$tryCatch@core/vendor/es6-promise/dist/es6-promise.js:331:24\nlib$es6$promise$$internal$$invokeCallback@core/vendor/es6-promise/dist/es6-promise.js:343:52\nlib$es6$promise$$internal$$publish@core/vendor/es6-promise/dist/es6-promise.js:314:52\nlib$es6$promise$asap$$flush@core/vendor/es6-promise/dist/es6-promise.js:125:17\nExpected false to equal true.\napps/files_versions/tests/js/versionmodelSpec.js:77:43\ncore/vendor/jquery/dist/jquery.min.js:2:56437\nj@core/vendor/jquery/dist/jquery.min.js:2:27194\nfireWith@core/vendor/jquery/dist/jquery.min.js:2:54761\ncore/vendor/jquery/dist/jquery.min.js:2:56856\ncore/js/files/client.js:9:30735\nlib$es6$promise$$internal$$tryCatch@core/vendor/es6-promise/dist/es6-promise.js:331:24\nlib$es6$promise$$internal$$invokeCallback@core/vendor/es6-promise/dist/es6-promise.js:343:52\nlib$es6$promise$$internal$$publish@core/vendor/es6-promise/dist/es6-promise.js:314:52\nlib$es6$promise$asap$$flush@core/vendor/es6-promise/dist/es6-promise.js:125:17",
"str": "An error was thrown in afterAll\nExpected false to equal true.\napps/files_versions/tests/js/versionmodelSpec.js:76:47\ncore/vendor/jquery/dist/jquery.min.js:2:56437\nj@core/vendor/jquery/dist/jquery.min.js:2:27194\nfireWith@core/vendor/jquery/dist/jquery.min.js:2:54761\ncore/vendor/jquery/dist/jquery.min.js:2:56856\ncore/js/files/client.js:9:30735\nlib$es6$promise$$internal$$tryCatch@core/vendor/es6-promise/dist/es6-promise.js:331:24\nlib$es6$promise$$internal$$invokeCallback@core/vendor/es6-promise/dist/es6-promise.js:343:52\nlib$es6$promise$$internal$$publish@core/vendor/es6-promise/dist/es6-promise.js:314:52\nlib$es6$promise$asap$$flush@core/vendor/es6-promise/dist/es6-promise.js:125:17\nExpected false to equal true.\napps/files_versions/tests/js/versionmodelSpec.js:77:43\ncore/vendor/jquery/dist/jquery.min.js:2:56437\nj@core/vendor/jquery/dist/jquery.min.js:2:27194\nfireWith@core/vendor/jquery/dist/jquery.min.js:2:54761\ncore/vendor/jquery/dist/jquery.min.js:2:56856\ncore/js/files/client.js:9:30735\nlib$es6$promise$$internal$$tryCatch@core/vendor/es6-promise/dist/es6-promise.js:331:24\nlib$es6$promise$$internal$$invokeCallback@core/vendor/es6-promise/dist/es6-promise.js:343:52\nlib$es6$promise$$internal$$publish@core/vendor/es6-promise/dist/es6-promise.js:314:52\nlib$es6$promise$asap$$flush@core/vendor/es6-promise/dist/es6-promise.js:125:17"
}
PhantomJS 2.1.1 (Linux 0.0.0): Executed 984 of 1053 (skipped 4) ERROR (19.564 secs / 18.747 secs)

Is there an easy way to trigger another CI run?

skjnldsv · 2018-10-25T09:01:08Z

@caugner https://drone.nextcloud.com/nextcloud/server/11833/283

danxuliu · 2018-10-25T11:51:46Z

Looks like either a test artefact or an unrelated test failing

Unrelated; fix is in #12039 ;-)

MorrisJobke · 2018-10-25T12:04:17Z

Unrelated; fix is in #12039 ;-)

@caugner Mind to rebase on current master as the fix was merged 😉

MorrisJobke · 2018-10-25T12:05:08Z

@caugner Mind to rebase on current master as the fix was merged 😉

I just had a look and it is exactly the same stack trace -> merging.

MorrisJobke · 2018-10-25T12:06:20Z

@caugner Thanks for your contribution. You could have a look at other nice starter issues if you wish so: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

I will also invite you to the Nextcloud organization here ;)

rullzer added enhancement 3. to review Waiting for reviews labels Oct 17, 2018

rullzer requested review from ChristophWurst, MorrisJobke, danxuliu, jancborchardt and juliusknorr October 17, 2018 20:23

rullzer added this to the Nextcloud 15 milestone Oct 17, 2018

rullzer approved these changes Oct 17, 2018
View reviewed changes

MorrisJobke approved these changes Oct 18, 2018•
edited
Loading
View reviewed changes

MorrisJobke requested changes Oct 18, 2018
View reviewed changes

caugner commented Oct 19, 2018
View reviewed changes

Comment threadcore/js/js.js Outdated

caugner commented Oct 19, 2018
View reviewed changes

Comment threadcore/js/tests/specs/coreSpec.js Outdated

ChristophWurst approved these changes Oct 24, 2018
View reviewed changes

skjnldsv approved these changes Oct 24, 2018
View reviewed changes

caugner commented Oct 24, 2018
View reviewed changes

OC.getCanonicalLocale: fix tests
d5e142a
Signed-off-by: Claas Augner <git@caugner.de>

MorrisJobke merged commit 116268a into nextcloud:masterOct 25, 2018

caugner deleted the 7837-filesize-format-with-locale branch October 25, 2018 12:22

	expect(OC.getCanonicalLocale('de')).toEqual('de');
	localeStub.returns('de');
	expect(OC.getCanonicalLocale()).toEqual('de');

	expect(OC.getCanonicalLocale('az_Cyrl_AZ')).toEqual('az-Cyrl-AZ');
	localeStub.returns('az_Cyrl_AZ');
	expect(OC.getCanonicalLocale()).toEqual('az-Cyrl-AZ');

	expect(OC.getCanonicalLocale('zu')).toEqual('zu');
	localeStub.returns('zu');
	expect(OC.getCanonicalLocale()).toEqual('zu');

	expect(OC.getCanonicalLocale('de_DE')).toEqual('de-DE');
	localeStub.returns('de_DE');
	expect(OC.getCanonicalLocale()).toEqual('de-DE');

Uh oh!

Uh oh!

Conversation

caugner commented Oct 17, 2018

Uh oh!

caugner commented Oct 17, 2018

Uh oh!

rullzer left a comment

Choose a reason for hiding this comment

Uh oh!

rullzer commented Oct 17, 2018

Uh oh!

caugner commented Oct 17, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kesselb commented Oct 18, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChristophWurst commented Oct 18, 2018

Uh oh!

Uh oh!

MorrisJobke left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caugner commented Oct 18, 2018

Uh oh!

caugner commented Oct 18, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caugner commented Oct 18, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rullzer commented Oct 19, 2018

Uh oh!

caugner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

caugner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skjnldsv commented Oct 19, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caugner commented Oct 19, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rullzer commented Oct 24, 2018

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

caugnerOct 24, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caugnerOct 24, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caugner left a comment

Choose a reason for hiding this comment

Uh oh!

caugnerOct 24, 2018

Choose a reason for hiding this comment

Uh oh!

caugnerOct 24, 2018

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Oct 24, 2018

Uh oh!

caugner commented Oct 25, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skjnldsv commented Oct 25, 2018

Uh oh!

danxuliu commented Oct 25, 2018

Uh oh!

caugner commented Oct 17, 2018•
edited
Loading

kesselb commented Oct 18, 2018•
edited
Loading

MorrisJobke left a comment •
edited
Loading

caugner commented Oct 18, 2018•
edited
Loading

caugner commented Oct 18, 2018•
edited
Loading

skjnldsv commented Oct 19, 2018•
edited
Loading

caugner commented Oct 19, 2018•
edited
Loading

caugnerOct 24, 2018•
edited
Loading

caugnerOct 24, 2018•
edited
Loading

caugner commented Oct 25, 2018•
edited
Loading