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
Numbro 2.0.0 #305
Conversation
@NicolasPetton @herodrigues @gwynjudd @Ben305 it's not ready yet, but if you want to have a look 😄 |
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 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. |
@ArmorDarks thanks for the input 😄 I know I've never tried |
Gruntfile.js
Outdated
verbose: true, // If you need output with rule names http://jscs.info/overview.html#verbose | ||
validateIndentation: 4 | ||
} | ||
all: ["tests-nodeunits/**/*.js"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks 😄
I usually try to use gulp as much as I can, but I don't really know why 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}})); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my. This one just cries to move directly into scripts, since you're basically hacking Gulp to make it work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 :)
gulpfile.js
Outdated
|
||
// Release | ||
|
||
gulp.task("release", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already have npm publish
. Really, there is no need to replicate it functionality, since it does same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just took the grunt tasks and mimic them 😄
It looks like all of that really need some love 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try to provide some ❤️ :)
@BenjaminVanRyseghem nice! I'll take a closer look tomorrow. I can help to finish those TODO tasks. |
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.
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. |
Also, if CI build time worries you, I'd recommend to switch to CircleCI. I can help with configuration. |
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... |
bb92d81
to
0df4e4e
Compare
src/validating.js
Outdated
forceSign: "boolean" | ||
}; | ||
|
||
const validLanguage = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I will give a shot to Validate.js I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, just mention me if you'll come to that, I'll be glad to help.
872fd7c
to
4cc780b
Compare
@BenjaminVanRyseghem what can be done about this task |
@herodrigues If you have a look at |
8b4d17e
to
c2d6e32
Compare
801cb39
to
02cc991
Compare
- replace grunt by gulp - replace jshint by eslint - use jasmine for tests - add integration tests support
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)
0850b51
to
902acf3
Compare
@BenjaminVanRyseghem or others, is there any migration guide from v1 to v2? |
@Carniatto this library is a bit outdated and it hasn't been maintained for a while. If you just want to perform simple format/unformat operations, then you can achieve that by using plain JavaScript. |
ouch
v2 should be fully backward compatible to v1, so no stress migrating. If that's not the case, I'll be glad to help you. |
@BenjaminVanRyseghem I didn't mean to be rude. The last feature PR 014d56c is indeed a bit old in terms of JS programming. Also, there are open issues from 2018. |
a full rewrite of numbro
Todo:
unformat
dist
testsNew features:
spaceSeparated
: should a space be inserted between the number and an abbreviation/currency symbolcurrencyDefaults
: defaults value for formatting currencybyteDefaults
: defaults value for formatting byteordinalDefaults
: defaults value for formatting ordinalpercentDefaults
: defaults value for formatting percentthousandsSize
: should that be renamedcharacteristicGroupSize
?unformat
is now a static method instead of an instance method)