Add `locale` option #39

meisterlampe · 2017-11-29T15:59:42Z

This is meant to close #4 (I copied the parameters for .toLocaleString(..) from @silverwind , who opened the issue)

Uses .toLocaleString(...) to create the human readable strings for a locale.
Specifying the locale is optional.
If ommited, the default language will be used.

Please add your thoughts on this. It was only a first shot.

silverwind · 2017-11-29T16:38:01Z

index.js

@@ -17,7 +17,7 @@ module.exports = num => {
 	}

 	const exponent = Math.min(Math.floor(Math.log10(num) / 3), UNITS.length - 1);
-	const numStr = Number((num / Math.pow(1000, exponent)).toPrecision(3));
+	const numStr = (num / Math.pow(1000, exponent)).toLocaleString(locale, { minimumFractionDigits: 2, maximumFractionDigits: 2 });


You can just use argument-less .toLocaleString() which will automatically format in the user's current locale, which I think is what 99% of users want.

Also, keep the Number().

silverwind · 2017-11-29T16:39:55Z

index.js

@@ -1,7 +1,7 @@
 'use strict';
 const UNITS = ['B', 'kB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB', 'YB'];

-module.exports = num => {
+module.exports = (num, locale = undefined) => {


I imagined a boolean option like localize which enables .toLocaleString(). I'd say default it to false.

I want to pass the locale, because I need to enable the user to change the language during runtime in my ionic-app.

What do you think about using your approach (boolean option localize, default to false) plus adding an option to set the locale to use in case of localize? (This is how moment.js does it)

Example for default locale de:

> prettybytes(1230000); 1.23 MB > prettybytes(1230000, true); // Default system locale 'de' is used 1,23 MB > prettybytes.locale('rn'); // Changes the locale for all subsequent calls with localize = true; > prettybytes(1230000, true): // Locale 'rn' is used 1.23 MB

Just to make xo linter happy :)

Some tests failed indicating, that no minimumFractionDigits are wanted. E.g.: 10.1 should be 10.1 and not 10.10

Node 4 doesn't support them (Travis CI build failed because of that). Default value isn't needed though, because checking for localize should work nevertheless. E.g.: - undefined == true => false - false == true => false - 0 == true => false - true == true => true - 1 == true => true

silverwind · 2017-11-30T15:46:40Z

index.js

@@ -1,7 +1,7 @@
 'use strict';
 const UNITS = ['B', 'kB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB', 'YB'];

-module.exports = num => {
+module.exports = (num, localize) => {


Forgot to mention this last time, but could you make an option object as the second argument, e.g. {localize: true}.

silverwind · 2017-11-30T15:47:30Z

LGTM once the option object is in. I think we can also do some unit tests for various locales (have to verify they work on Travis), but I can add those after merging this one.

meisterlampe · 2017-12-01T08:45:20Z

Added an options object.

In my case I need to pass in the locale-key as I change the language during runtime in an app using Ionic. Any chance to add this in another PR later on?

kevva · 2017-12-01T10:02:58Z

index.js

@@ -17,7 +17,8 @@ module.exports = num => {
 	}

 	const exponent = Math.min(Math.floor(Math.log10(num) / 3), UNITS.length - 1);
-	const numStr = Number((num / Math.pow(1000, exponent)).toPrecision(3));
+	const numTmp = Number((num / Math.pow(1000, exponent)).toPrecision(3));
+	const numStr = options && options.localize ? numTmp.toLocaleString() : numTmp;


Maybe do options = Object.assign({}, options) after the Number.isFinite(num) if statement at the start of the function. This ensures that options is an object too.

Note that Object.assign required Node.js 5 or greater which breaks compatibilty on the current minimum version 4. I'd just to opts = opts || {} on the start of the function.

@silverwind Object.assign is supported on Node.js 4

Uh, you're right, carry on. (I was misinterpreting http://node.green/)

Sorry for the late reply. You're right. Using object.assign is way better than checking for undefined on each use. I'll add this.

silverwind · 2017-12-01T15:05:18Z

In my case I need to pass in the locale-key

Then let's change the option name to locale and typecheck it. On string, pass it to toLocaleString, otherwise use it as a boolean which decides on whether to use toLocaleString.

meisterlampe · 2017-12-04T12:30:20Z

That would be a good solution for me. Thanks. I will add it.

Don't check options on each use. Check it at the beginning of the method instead.

If locale = true => localize using the default locale If locale = string => the value is expected to be a locale-key-string and is used as parameter for toLocaleString(..)

...and use single quoted string. To pass linter checks

Removed separate check of options.local. typeof has no problems with undefined-arguments

meisterlampe · 2017-12-11T08:05:46Z

@kevva Are you fine with the changes?

sindresorhus · 2017-12-11T12:55:21Z

I'm fine with the change, but it needs docs and tests.

See https://github.com/sindresorhus/boxen#api for how to format the docs.

meisterlampe · 2017-12-14T08:41:57Z

OK I'll look into that.

Don't know, how that got into the code :)

This should fix the failing test `localized output`

meisterlampe · 2017-12-19T15:20:11Z

Added those lines, but it still isn't working (same error during test-execution).
Travis-CI build says: libicu-dev is already the newest version (52.1-3ubuntu0.7). So maybe, this had no effect. (https://travis-ci.org/sindresorhus/pretty-bytes/jobs/318665413)
Any other commands I can try? I wasn't able to find any useful info using google (beside building node with a special flag).

silverwind · 2017-12-19T16:59:53Z

I assume Travis installed the official Node.js packages, so I checked them in both Ubuntu 14.04 and 16.04 and I discovered that both are actually built with small-icu, not system-icu, which means it won't detect a system ICU library at all. We can use the full-icu module thought, try:

before_install:
  - npm install full-icu
env:
  - NODE_ICU_DATA=node_modules/full-icu

You're still missing a option description in README.md, I'd note there that the option generally works in browsers, but for Node, the user has to make sure that ICU is working, probably by linking them to full-icu.

meisterlampe · 2017-12-20T08:06:58Z

Hm.. the environment is set before installing the npm package and with the variable set, installing seems to error out. (https://travis-ci.org/sindresorhus/pretty-bytes/jobs/319039889)
I tried to export the variable in after_install, but had no luck (https://travis-ci.org/sindresorhus/pretty-bytes/jobs/319047209)

silverwind · 2017-12-20T21:00:24Z

There's no after_install step according to this, try exporting in a before_script.

because there is no after_install step :)

silverwind · 2017-12-21T08:09:19Z

Ah, it finally passed. Can you document the new option in the README and add a note that users may need to install the full-icu package for it to work?

meisterlampe · 2017-12-21T08:59:11Z

You were right.. there is no `after_install. Ouch. Thanks for the hint on this :)
I tried to document the new option in the readme. I'm happy to get corrections, if something is missing, not well verbalized or just a typo

silverwind · 2017-12-21T20:57:50Z

index.js

 	const neg = num < 0;

 	if (neg) {
 		num = -num;
 	}

 	if (num < 1) {
+		num = toLocaleString(num, options.locale);


Very minor, but I'd declare let numStr earlier and then assign it here, making it clearer that num is always a number.

I agree. Also changed line 24, where the result is always a number and not a numStr

silverwind · 2017-12-21T21:06:20Z

readme.md

+
+**Note:** Localization should generally work in browsers. Node-Users have to ensure, there Node version has [full ICU-Support](https://github.com/nodejs/node/wiki/Intl):
+- if your using linux, node should be able to link to the installed system-icu automatically
+- or you install the [full-icu-package](https://github.com/unicode-org/full-icu-npm) and set the environment variable [NODE-ICU-DATA](https://github.com/nodejs/node/wiki/Intl#using-and-customizing-the-small-icu-build) to `node_modules/full-icu`


It's not entirely true that all node versions need the module. The Node.js on macOS' homebrew for example comes built with system-icu. I'd word it like this:

Localization should generally work in browsers. Node.js needs to be built with full-icu or system-icu. Alternatively, the full-icu module can be used to provide support at runtime.

Added your wording.

silverwind · 2017-12-23T19:13:08Z

LGTM, this should be ready to merge, @sindresorhus.

meisterlampe · 2018-01-02T13:00:10Z

It would be nice, if you could merge this. If something is missing, I'm happy to add/fix it.

silverwind · 2018-01-09T01:32:27Z

ping @sindresorhus and @kevva.

meisterlampe · 2018-01-16T13:33:14Z

re-ping @sindresorhus and @kevva :)

sindresorhus · 2018-01-22T20:40:32Z

index.js

 	if (num < 1) {
-		return (neg ? '-' : '') + num + ' B';
+		numStr = toLocaleString(num, options.locale);


const numStr = toLocaleString(num, options.locale);

sindresorhus · 2018-01-22T20:41:27Z

index.js

 	}

 	const exponent = Math.min(Math.floor(Math.log10(num) / 3), UNITS.length - 1);
-	const numStr = Number((num / Math.pow(1000, exponent)).toPrecision(3));
+	num = Number((num / Math.pow(1000, exponent)).toPrecision(3));
+	numStr = toLocaleString(num, options.locale);


const numStr = toLocaleString(num, options.locale);

sindresorhus · 2018-01-22T20:41:41Z

index.js

+/**
+ * Formats the given number using number.toLocaleString(..).
+ * If locale is a string, the value is expected to be a locale-key (e.g. 'de').
+ * If locale is true or 1, the system default locale is used for translation.


If locale is true or 1

Should only be when it's true.

sindresorhus · 2018-01-22T20:41:56Z

index.js

+	let result = num;
+	if (typeof locale === 'string') {
+		result = num.toLocaleString(locale);
+	} else if (locale) {


} else if (locale === true) {

meisterlampe · 2018-01-24T09:21:10Z

@sindresorhus: Thanks for your feedback. I agree with the requested changes and added them to the PR.

meisterlampe · 2018-02-06T16:10:49Z

@sindresorhus and @kevva: Can you merge this please? If something is missing, I'm happy to fix things

meisterlampe · 2018-03-05T17:19:28Z

Can this be merged please? I would like to remove my forked version from my project.

silverwind · 2018-03-07T16:52:38Z

Not sure what the holdup here is.

One suggestion for the full-icu dependency: Add it under both peerDependencies and devDependencies. That way, users will notice it when installing dependencies and it will be included for developers so npm test can pass locally for them, while also making sure it's not included in browser builds.

Oh, and you could make sure that environment variable is set during npm test by using cross-env.

These two changes should make the adjustments in travis.yml unneccesary.

sindresorhus · 2018-03-07T17:36:33Z

Not sure what the holdup here is.

Just time. I have over 300 PRs to review…

Add it under both peerDependencies and

I don't want it under peerDependencies. It could make people think it's a requirement.

meisterlampe · 2018-03-07T19:15:51Z

Not sure what the holdup here is.
Just time. I have over 300 PRs to review…

That is ok.. Just thought, this PR has been forgotten.
Take your time and thanks for voluntarily maintaining so many open source projects.

silverwind · 2018-03-07T21:56:12Z

I don't want it under peerDependencies. It could make people think it's a requirement.

Hmm okay. We should still have it in devDependencies so tests pass unconditionally.

On the topic of ICU, I meant to do some research on the binary sizes and propose building Node.js with full ICU by default, so hopefully this dependency won't be required some day.

Edit: see nodejs/node#19214

meisterlampe added 2 commits November 29, 2017 16:49

make use of .toLocaleString

9fabf31

Casting to number is not needed

4e542fb

silverwind reviewed Nov 29, 2017

View reviewed changes

meisterlampe added 5 commits November 30, 2017 08:20

Add number again

962d205

Remove whitespace after '{' and before '}'

6634535

Just to make xo linter happy :)

Just use maximumFractionDigits

06ce04e

Some tests failed indicating, that no minimumFractionDigits are wanted. E.g.: 10.1 should be 10.1 and not 10.10

Default to not using toLocaleString

de0ce66

silverwind reviewed Nov 30, 2017

View reviewed changes

Add options object as second parameter

a11f77d

kevva suggested changes Dec 1, 2017

View reviewed changes

meisterlampe added 5 commits December 4, 2017 13:36

Ensure options is an object

04ffac1

Don't check options on each use. Check it at the beginning of the method instead.

Remove trailing space

800d8db

Change localize to locale

3c6a5ed

If locale = true => localize using the default locale If locale = string => the value is expected to be a locale-key-string and is used as parameter for toLocaleString(..)

Removed whitespace (again :))...

873579d

...and use single quoted string. To pass linter checks

No need to check for existence of options.local

b415775

Removed separate check of options.local. typeof has no problems with undefined-arguments

silverwind approved these changes Dec 4, 2017

View reviewed changes

meisterlampe added 5 commits December 19, 2017 08:36

Add some tests

fd91fba

Replace . with ,

d42bb38

Don't know, how that got into the code :)

Fix linter warnings

27f1962

Added translation of numbers < 1

5111173

This should fix the failing test `localized output`

Fix linter warning: no-else-return

5daab78

meisterlampe added 2 commits December 20, 2017 08:31

Try to get full-icu support in travis-ci-build

9414871

set environment variable after install

94bf8b7

use before_script to export

7ce98a4

because there is no after_install step :)

Add locale-option-description to readme.md

8fa6494

silverwind reviewed Dec 21, 2017

View reviewed changes

meisterlampe added 2 commits December 22, 2017 09:00

Update readme.md

642c5ee

Assign output of toLocaleString to numStr, use num for numbers only

0f77a7c

sindresorhus requested changes Jan 22, 2018

View reviewed changes

meisterlampe added 2 commits January 23, 2018 15:50

explicitly check for true

2560023

use const numstr instead of shared let variable

cdf11bc

sindresorhus changed the title ~~Make use of .toLocaleString~~ Add locale option May 6, 2018

sindresorhus merged commit 73a7955 into sindresorhus:master May 6, 2018

Add locale option #39

Add locale option #39

Conversation

meisterlampe commented Nov 29, 2017

silverwind Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

meisterlampe Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind commented Nov 30, 2017 • edited Loading

meisterlampe commented Dec 1, 2017

kevva Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

silverwind Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meisterlampe Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

silverwind commented Dec 1, 2017 • edited Loading

meisterlampe commented Dec 4, 2017

meisterlampe commented Dec 11, 2017

sindresorhus commented Dec 11, 2017 • edited Loading

meisterlampe commented Dec 14, 2017

meisterlampe commented Dec 19, 2017 • edited Loading

silverwind commented Dec 19, 2017 • edited Loading

meisterlampe commented Dec 20, 2017 • edited Loading

silverwind commented Dec 20, 2017

silverwind commented Dec 21, 2017

meisterlampe commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind commented Dec 23, 2017

meisterlampe commented Jan 2, 2018

silverwind commented Jan 9, 2018

meisterlampe commented Jan 16, 2018

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

meisterlampe commented Jan 24, 2018

meisterlampe commented Feb 6, 2018

meisterlampe commented Mar 5, 2018

silverwind commented Mar 7, 2018 • edited Loading

sindresorhus commented Mar 7, 2018

meisterlampe commented Mar 7, 2018

silverwind commented Mar 7, 2018 • edited Loading

Add `locale` option #39

Add `locale` option #39

silverwind Nov 29, 2017 •

edited

Loading

silverwind Nov 29, 2017 •

edited

Loading

meisterlampe Nov 30, 2017 •

edited

Loading

silverwind commented Nov 30, 2017 •

edited

Loading

kevva Dec 1, 2017 •

edited

Loading

silverwind Dec 1, 2017 •

edited

Loading

meisterlampe Dec 4, 2017 •

edited

Loading

silverwind commented Dec 1, 2017 •

edited

Loading

sindresorhus commented Dec 11, 2017 •

edited

Loading

meisterlampe commented Dec 19, 2017 •

edited

Loading

silverwind commented Dec 19, 2017 •

edited

Loading

meisterlampe commented Dec 20, 2017 •

edited

Loading

silverwind Dec 21, 2017 •

edited

Loading

silverwind commented Mar 7, 2018 •

edited

Loading

silverwind commented Mar 7, 2018 •

edited

Loading