fix issue #120 - default values to be strings

[ericmorand]

Default values are now returned as strings instead of raw values when parameter default value
consists of a list of expressions. This is the expected behavior of the lib. Plus, the fix is now
much cleaner.
TAG: latest

fixes #120

[codecov]

Codecov Report

Merging #121 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #121   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines          96     93    -3     
=====================================
- Hits           96     93    -3

Impacted Files	Coverage Δ
src/lib/plugins/params.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05018b6...9a48bac. Read the comment docs.

[tunnckoCore]

Don't think those should be strings. I remember that i thought about that when built it. Wouldn't it be feel better to be actual values?

[tunnckoCore]

This one not feels good too.

[tunnckoCore]

I may agree that this behavior is good point too. But we can think for an option to allow they to be raw values - probably enabled by default, so you can use it to disable it and always get strings. It would be also backward compatible so no major version would be needed.

[tunnckoCore]

Hm. Okey, make a lot of sense. Delegate the casting entirely to the user. It also is more easier to know that the value can be only string or undefined.

[tunnckoCore]

@ericmorand, btw, i not really understand that list thing, and can't find any docs on it.

What's the point of that signature?

var bar = (a = (true, 3)) => console.log(a)
bar() // => 3

bar = (a = (null, 3)) => console.log(a)
bar() // => 3

bar = (a = (false, 3)) => console.log(a)
bar() // => 3

bar = (a = (1, 2, 3, 4, 5, 6)) => console.log(a)
bar() // => 6

It always picks the last one... Then what's the point of having the previous ones?

The whole thing really not make any sense to me. It seems like a tuple structure but isn't :D

[tunnckoCore]

And one more thing. Can you add one more test with (a = 1) => {} so we can verify that numbers are strings too.

[ericmorand]

About your first point I was surprized at first that your library did return strings as parameter default values instead of raw values.

And then I got used to it and I even think now that it's better that you don't try to evaluate the value. That keeps your lib super simple, fast and easily testable. I'm not sure what the impact of evaluating the value would be on performance but I think you should not take that kind of responsibility and let the lib consumer evaluate the default value when it needs it.

About the "list" thing, I'll have to find the spec.

But consider the following code:

console.warn(5), 10;

It's a perfectly valid expression. It executes console.warn(5) and then 10 - which does nothing if not part of an assignment.

Going further:

let foo = (console.warn(5), 10);

Will execute console.warn(5) and then assign 10 to foo since 10 is now part of an assignment. That's what happens with parameters default values.

Real life use case is questionable except for code instrumentation where it is awesome:

let i = 0;

function foo(a = (i++, 5)) {

}

foo();
foo(10);
foo(1);
foo();

console.warn(i); // display 2

As you can see, it is possible to know how many times the default value has been used. In this case only two times. Istanbul and other code coverage libs make an extensive usage of this. And seriously this is awesome.

I'll update the PR to add number test.

Thanks for your lib and your work. It makes my life so much easier on Twing.

[tunnckoCore]

I think you should not take that kind of responsibility and let the lib consumer evaluate if it needs it.

Yea, exactly.

Will execute console.warn(5) and then assign 10 to foo since 10 is now part of an assignment. That's what happens with parameters default values.

Oooh... really. 🤣 I forgot about that, cuz don't use it. I always forget that JS is pretty crazy and had tons of things that can make you love it and hate it :D

About the "list" thing, I'll have to find the spec.

No need. Seen few examples for default params, and seems like it evaluates them on call time. Even there's so crazy thing such as that next params can access previous param's values :D Don't now sounds me too crazy and too much. I've seen some very crazy examples and strongly believe it shouldn't be used so hard because leads to very ugly code. They are cool, but to some limits.

As you can see, it is possible to know how many times the default value has been used.

Sounds pretty fantastic, yes!

---

Thanks for the test. I should reorganized them cuz they are very boring and repeated. But yea, its pretty awesome to have such stability and to be used so much :)

[tunnckoCore]

I'll merge, once i consider a descriptive message, because everything is automatic with the publishing and releasing.

Also not sure to release major or minor (depends on the type in the merge commit message ;d). More thinking for a major for safety.

[ericmorand]

Well, this PR is only a bugfix - #110 created an inconsistency with the rest of the lib, so I think minor is appropriate. Expression assignment support is already part of the lib since 5.1.5 I think.

[ericmorand]

No need. Seen few examples for default params, and seems like it evaluates them on call time. Even there's so crazy thing such as that next params can access previous param's values :D Don't now sounds me too crazy and too much. I've seen some very crazy examples and strongly believe it shouldn't be used so hard because leads to very ugly code. They are cool, but to some limits.

I agree that outside of code instrumentation they should not be used as they lead to some very ugly code. Plus I fail to see a real-word use case that can't be solved by something cleaner.

But since instrumenters use that kind of assignment, parse-function should support it. Without that support, projects that use parse-function to retrieve parameter default values become impossible to instrument and thus can't be code covered.

For example, assuming you're using parse-function@5.1.1:

let foo = function(a = 5) {
}

let parseFunction= require('parse-function');
let parser = parseFunction();

let functionDef = parser.parse(foo);
let defaults = functionDef.defaults

return defaults;

This code works perfectly and returns the defaults parameters of the function. Run it via a test suite and it will succeed.

But try it with code-coverage added and the test suite will crash. Because the instrumenter would have changed the code to:

let foo = function(a = (cov1234++, 5)) {
}

let parseFunction= require('parse-function');
let parser = parseFunction();

let functionDef = parser.parse(foo);
let defaults = functionDef.defaults

return defaults;

That's why I proposed #110 and the belonging PR. To make your lib usable in real-case scenarios, where we need to add code coverage to our projects. :)

[tunnckoCore]

The problem is that someone may depend on that the default values are actual values and not strings, so merging that will crash his project. But yup, probably minor make sense would be enough.

Thanks again! 🍺

[tunnckoCore]

Merged. 🎉

But damn Travis.. has some hard hang ;/

edit: wasn't best time to merge. Some log db outages probably, seen some thread about that from previous year ;d

I in anyway will switch to Circle and will updated the contrib flow, but want first to release v5.2 and then the next version will be major and requiring Node >=8.6

[tunnckoCore]

Finally released and published as v5.2.0 :)

[ericmorand]

I'm sorry to bother you but are you sure your package is correct?

[tunnckoCore]

yeaa.. forgot to run the build before publish... "kill all humans". :D