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

Numbro 2.0.0 #305

Merged
merged 3 commits into from Jan 15, 2018

Conversation

Projects
None yet
3 participants
@BenjaminVanRyseghem
Owner

BenjaminVanRyseghem commented Aug 12, 2017

a full rewrite of numbro

  • use es6
  • split up code into modules
  • use eslint
  • use browserify
  • use gulp
  • use jasmine
  • use istanbul

Todo:

  • implement a format parser
  • more parsing-tests tests
  • add BigNumber support
  • rewrite all languages files
  • fix language loading
  • implement unformat
  • add documentation
  • fix todos
  • update the website
  • add "format as exponent XXeXX"
  • add dist tests

New features:

  • the provided format can now be an object
  • defaults are now language specific and not global anymore
  • new options to language files:
    • spaceSeparated: should a space be inserted between the number and an abbreviation/currency symbol
    • currencyDefaults: defaults value for formatting currency
    • byteDefaults: defaults value for formatting byte
    • ordinalDefaults: defaults value for formatting ordinal
    • percentDefaults: defaults value for formatting percent
    • thousandsSize: should that be renamed characteristicGroupSize?
  • cleanup of the API (unformat is now a static method instead of an instance method)
@BenjaminVanRyseghem

This comment has been minimized.

Owner

BenjaminVanRyseghem commented Aug 12, 2017

@NicolasPetton @herodrigues @gwynjudd @Ben305 it's not ready yet, but if you want to have a look 😄

@ArmorDarks

This comment has been minimized.

ArmorDarks commented Aug 12, 2017

Wow, quite cool!

Didn't look into code yet, though...

Just for the information, it might be worth to look at Standard as a replacement for pure ESLint, since it comes with reasonable styling and more powerful auto --fix , have much more built-in features than ESLint-only solution and remove the burden of maintaining its own ESLint config.

Another alternative is Prettier coupled with Prettier-based ESLint config.

Also, it is a matter of taste, but most recent Jest and it's watch mode, snapshot testing, built-in coverage are really cool. We've used it on our projects and never looked back.

@BenjaminVanRyseghem

This comment has been minimized.

Owner

BenjaminVanRyseghem commented Aug 12, 2017

@ArmorDarks thanks for the input 😄

I know Standard and Prettier but I tend to find them too strict. I like that eslint can be really customized and extended.

I've never tried Jest, I will have a look 😄

verbose: true, // If you need output with rule names http://jscs.info/overview.html#verbose
validateIndentation: 4
}
all: ["tests-nodeunits/**/*.js"]

This comment has been minimized.

@ArmorDarks

ArmorDarks Aug 12, 2017

Grunt is cool, but you really do not need it for this task.

Instead, it is much easier to run nodeunit directly from scripts in package.json

"scripts": {
  "test": "nodeunit tests-nodeunits/**/*.js"
}

and then run in console

npm test

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Aug 12, 2017

Owner

grunt is to be removed, I kept it so far to run old tests 😄

it's already replaced with gulp

gulpfile.js Outdated
.pipe(plugins.eslint())
.pipe(plugins.eslint.format("unix"))
.pipe(plugins.eslint.failAfterError());
});

This comment has been minimized.

@ArmorDarks

ArmorDarks Aug 12, 2017

Here it comes again. Gulp is overkill for that task.

"scripts": {
  "lint": "eslint \"src/**/*.js,tests/**/*.js,languages/**/*.js\"",
  "test": "lint && nodeunit tests-nodeunits/**/*.js"
}

and then run in console

npm test

With Standard this might be even simpler:

"scripts": {
  "lint": "standard",
  "test": "lint && nodeunit tests-nodeunits/**/*.js"
}

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Aug 12, 2017

Owner

thanks 😄

I usually try to use gulp as much as I can, but I don't really know why 😄

This comment has been minimized.

@ArmorDarks

ArmorDarks Aug 12, 2017

There is really no point in doing that, since you basically keep adding additional abstraction layers for really simple things.

Do not mistake me. I have very large experience with Grunt (Kotsu) and it took us some time too to realize, that by doring everything exclusively through Grunt\Gulp we only make things harder, dealing with abstractions, and complicating things when it comes to CI integration.

Grunt and Gulp are cool, but they are for more difficult tasks, really.

return gulp.src(["./src/**/*.js", "./languages/**/*.js"])
.pipe(plugins.istanbul())
.pipe(plugins.istanbul.hookRequire());
});

This comment has been minimized.

@ArmorDarks

ArmorDarks Aug 12, 2017

This one better to live in scripts too, Gulp here doesn't help at all

However, with Jest it would be even easier, since Istanbul comes as part of Jest:

"scripts": {
  "lint": "eslint \"src/**/*.js,tests/**/*.js,languages/**/*.js\"",
  "test": "lint && jest --coverage && nodeunit tests-nodeunits/**/*.js"
}
gulpfile.js Outdated
}))
.pipe(plugins.istanbul.writeReports());
// .pipe(plugins.istanbul.enforceThresholds({thresholds: {global: 90}}));
});

This comment has been minimized.

@ArmorDarks

ArmorDarks Aug 12, 2017

This one could be moved to scripts too, effectively reducing dependency on 3rd-party plugins and eleminating unnecessary abstraction.

gulpfile.js Outdated
// Coverage
gulp.task("basic-coverage", () => {
return gulp.src("").pipe(plugins.shell("istanbul cover ./node_modules/.bin/jasmine --captureExceptions")

This comment has been minimized.

@ArmorDarks

ArmorDarks Aug 12, 2017

Oh my. This one just cries to move directly into scripts, since you're basically hacking Gulp to make it work.

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Aug 12, 2017

Owner

ahah 😄

I think all of that is mainly copy/pasted without too much thinking (also it was made in a train during a 7h trip 😄 )

This comment has been minimized.

@ArmorDarks
gulpfile.js Outdated
// Release
gulp.task("release", () => {

This comment has been minimized.

@ArmorDarks

ArmorDarks Aug 12, 2017

You already have npm publish. Really, there is no need to replicate it functionality, since it does same thing.

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Aug 12, 2017

Owner

I just took the grunt tasks and mimic them 😄

It looks like all of that really need some love 😉

This comment has been minimized.

@ArmorDarks

ArmorDarks Aug 12, 2017

I try to provide some ❤️ :)

@herodrigues

This comment has been minimized.

Contributor

herodrigues commented Aug 12, 2017

@BenjaminVanRyseghem nice! I'll take a closer look tomorrow. I can help to finish those TODO tasks.

@ArmorDarks

This comment has been minimized.

ArmorDarks commented Aug 12, 2017

I know Standard and Prettier but I tend to find them too strict. I like that eslint can be really customized and extended.

Well, that's the point of such tools. It's just widely accepted by community standards, you just plug them and stop worrying about all those little things. But yeap, it takes some time to embrace them, and at first it was hard for us to submit to some of their choices too.

But after all, it makes all JS codebase much more uniform. Anyway, it is up to you. I'm just mentioning.

I've never tried Jest, I will have a look

Jest is basically Jasmin on steroids. At first it was completely Jasmin-based, but now they switched to their own engine due to performance limitations of Jasmine, but API mostly same with some cool additions, like snapshots testing. And you really should check their watch. It is really cool.

But in same time since they interchangeable, this isn't really critical choice. Jasmin is cool. Jest too, but tries to be cooler. And it is trendy. Choose whatever your heart desires.

@ArmorDarks

This comment has been minimized.

ArmorDarks commented Aug 12, 2017

Also, if CI build time worries you, I'd recommend to switch to CircleCI. I can help with configuration.

@BenjaminVanRyseghem

This comment has been minimized.

Owner

BenjaminVanRyseghem commented Aug 12, 2017

But in same time since they interchangeable, this isn't really critical choice. Jasmin is cool. Jest too, but tries to be cooler. And it is trendy. Choice whatever your heart desires.

I had a quick look, it indeed looks close, but the mocking library is a bit different. And honestly I don't feel like rewriting all the tests again 😄

@ArmorDarks

This comment has been minimized.

ArmorDarks commented Aug 12, 2017

I had a quick look, it indeed looks close, but the mocking library is a bit different. And honestly I don't feel like rewriting all the tests again 😄

I don't think that most part of tests will require any rewriting, but I can fix those which will require it.

But yeap, mocking library is indeed different. And I don't know how much is it different, since I'm trying not to use mocking at all, thus never worked with Jest-one.

Well, if you'll decide that switching to Jest worth it, I can look into mocking part and decide, is it easily portable or not. Meanwhile, will learn Jest's mocking capabilities better...

forceSign: "boolean"
};
const validLanguage = {

This comment has been minimized.

@ArmorDarks

ArmorDarks Aug 13, 2017

Too late for me, but I will mention it anyway.

It would be more logical to use Flow for those checks, it will greatly reduce amount of code.

If you're not ok with Flow, there is also a great library tcomb. To be honest, I've been completely charmed with its elegancy and how refinements works.

Here is example of tcomb validation.

One of the side benefits is that tcomb can be used during runtime (on contrary to Flow), and in most desperate situations there is a babel plugin which transpires Flow to tcomb.

Anyway, it is probably too late, but I'd stand for already implemented validation solution, since it will be much easier to contribute to it for newcomers (because, well, of well-known technologies).

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Aug 13, 2017

Owner

Flow and tcomb seems to be type checkers, where here we need to validate more that just the type, so I am not sure it will fit

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Aug 13, 2017

Owner

But I will give a shot to Validate.js I think

This comment has been minimized.

@ArmorDarks

ArmorDarks Aug 14, 2017

Yeap, Flow is a type checker, but you can validate much more with it.

Tcomb is slightly more. It is type checker, but also a validator. With it refinements you can validate almost anything based on provided types, and API for refinements is very elegant. In some ways, it is much more flexible than Flow, but they are just for slightly different purposes, since Flow is static type checker, while tcomb is for runtime type checking, validation and introspection. I'd say, usually they live quite good together.

Here is example with some tcomb refinaments, which validates much more complex things. Just for getting impression how flexible it is.

I don't want to push on you any tools, for sure, sorry if it looks like this. Just sharing my experience — my life before starting using tcomb been definitely harder, since I wasted a lot of time for writing boilerplate just to validate what I needed.

Regarding Validate.js — it is quite cool, but for me it was too heavy, with too much things I'd never need. Oops, I've actually mixed up Validate.js with Joi. Well, we discarded Validate.js just because writing precise refinements which suited our needs for tcomb was easier and felt more powerful, more flexible.

It might be also worth check this issue LotusTM/Kotsu#165 where I conducted some research on possible validation solutions. Our internal discussions, unfortunately, didn't make it to that issue, but at least you can see list of most popular solutions.

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Aug 14, 2017

Owner

I don't want to push on you any tools, for sure, sorry if it looks like this. Just sharing my experience — my life before starting using tcomb been definitely harder, since I wasted a lot of time for writing boilerplate just to validate what I needed.

Sorry if I gave you this impression... I am glad you're sharing your experience here 😄 I will definitely dig into this. Maybe we could create a ticket to discuss that further more?

This comment has been minimized.

@ArmorDarks

ArmorDarks Aug 14, 2017

Sure, just mention me if you'll come to that, I'll be glad to help.

@herodrigues

This comment has been minimized.

Contributor

herodrigues commented Aug 19, 2017

@BenjaminVanRyseghem what can be done about this task rewrite all languages files?

@BenjaminVanRyseghem

This comment has been minimized.

Owner

BenjaminVanRyseghem commented Aug 19, 2017

@herodrigues If you have a look at src/en-US.js you will see the format changed a bit.
We need to adjust all the other language files.

BenjaminVanRyseghem added some commits Nov 22, 2017

Rewrite numbro boilerplate
- replace grunt by gulp
- replace jshint by eslint
- use jasmine for tests
- add integration tests support
Complete rewrite of numbro
It keeps backward compatibility as much as possible

Breaking feature:

- previously, not specifying a mantissa size meant to have none, now it
means to keep it untouched:

    numbro(100.12).format("0")` // -> `"100.12"`

New features:

- the provided format can/should now be an object
- defaults are now language specific and not global anymore
- new options to language files:
    - spaceSeparated: should a space be inserted between the number and an abbreviation/currency symbol
    - currencyDefaults: defaults value for formatting currency
    - byteDefaults: defaults value for formatting byte
    - ordinalDefaults: defaults value for formatting ordinal
    - percentDefaults: defaults value for formatting percent
    - thousandsSize: how many digits per group in the characteristic
- cleanup of the API (unformat is now a static method instead of an instance method)

@BenjaminVanRyseghem BenjaminVanRyseghem merged commit b5c4b63 into develop Jan 15, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@BenjaminVanRyseghem BenjaminVanRyseghem deleted the numbro2.0 branch Jan 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment