add config.getInt(filename, default_value) #44
Conversation
Codecov Report
@@ Coverage Diff @@
## master #44 +/- ##
==========================================
+ Coverage 81.54% 82.02% +0.48%
==========================================
Files 8 8
Lines 428 434 +6
Branches 124 126 +2
==========================================
+ Hits 349 356 +7
+ Misses 79 78 -1
Continue to review full report at Codecov.
|
| const r = parseInt(cfreader.read_config(full_path, 'value', null, null), 10); | ||
|
|
||
| if (!isNaN(r)) return r; | ||
| return parseInt(default_value, 10); |
There was a problem hiding this comment.
What about possibility to set as default "null" if value is not present?
There was a problem hiding this comment.
Why null instead of NaN?
The results of the function should be as easy to deal with as possible. Per JS conventions, if you pass in something that can't be coerced into a number, you get a NaN. Anything else can be counted on to be an integer. If we're going to return something else, we should have a very good reason.
There was a problem hiding this comment.
I can see the argument for null: If you have no config set, you can know it wasn't set. Of course you should handle that through defaults via the param, but often we don't/won't, because we're often dumb, so instead people will do: config.getInt('thing') || some_default.
It's hard to argue for allowing people to be dumb though. But the NaN issue is painful if people expect a non-set config to be falsy.
Of course I just typed all that, and then decided to test it, and now I found out I'm wrong:
$ node
> parseInt('fool');
NaN
> parseInt('fool') || 10;
10
So basically ignore my entire comment and the stuff I wrote on the other ticket that inspired this also.
|
getBool and getString might be also handy |
No arguments, but some use cases should show up before we add them. |
| const r = parseInt(cfreader.read_config(full_path, 'value', null, null), 10); | ||
|
|
||
| if (!isNaN(r)) return r; | ||
| return parseInt(default_value, 10); |
There was a problem hiding this comment.
I can see the argument for null: If you have no config set, you can know it wasn't set. Of course you should handle that through defaults via the param, but often we don't/won't, because we're often dumb, so instead people will do: config.getInt('thing') || some_default.
It's hard to argue for allowing people to be dumb though. But the NaN issue is painful if people expect a non-set config to be falsy.
Of course I just typed all that, and then decided to test it, and now I found out I'm wrong:
$ node
> parseInt('fool');
NaN
> parseInt('fool') || 10;
10
So basically ignore my entire comment and the stuff I wrote on the other ticket that inspired this also.
|
Not usually a fan of combined tickets like this, but I'll let this one pass as it's fairly inconsequential. |
I was asked to adjust |
|
Lets put some meat on that guess: Not my definition of a lot, but there are a few. Instead of adding a
As before, since |
|
You are right I've made assumptions before proper research. (and I'am scared with specialities of JS type system also) |
|
No description provided.