Closed briancavalier closed 7 years ago
LGTM.
It's not clear to me why coverage isn't 100% after 4746514, and the html coverage isn't any help :/ If you look at the diff with master, only the new namespaced methods have been added. Since they just just immediately call existing code (which was already at 100% coverage), they should be fully covered simply by calling them at least once somewhere in the test suite, which is what 4746514 does.
If you look back at this coveralls comment, which is just before 4746514, then commit where I added the laws. Then, look at this one, after that commit, the coverage percentage is exactly the same. IOW, the fantasy land laws, somehow, are simply not being counted by coveralls.
I'll try a test where I explicitly call one of the new methods in a test directly, rather than letting a fantasyland law call it, and see if that helps.
That seems to be the problem: istanbul isn't reporting code covered by the fantasy land laws. (perhaps since they are outside of the dirs being instrumented?)
Here's what local istanbul reports for me in this PR branch:
Statements : 99.27% ( 813/819 )
Branches : 89.96% ( 206/229 )
Functions : 97.38% ( 223/229 )
Lines : 99.24% ( 784/790 )
And here's what it reports for me when I use a hand-coded Functor identity test instead of delegating to fantasy-land/laws/functor.
Statements : 99.39% ( 814/819 )
Branches : 89.96% ( 206/229 )
Functions : 97.82% ( 224/229 )
Lines : 99.37% ( 785/790 )
I guess an alternative is to write our own simple set of tests for the FL namespaced methods in addition to using the FL test suite. I was hoping just to rely on the FL suite, tho.
Haha, lovely. Fantasy Land's laws node modules require a version of node that supports destructuring. Since node 6 is LTS now, we could drop node 4 & 5.
See #87
Now that FL 2.1 is released, it might be more correct to implement Alt (currently concat
, i.e. race
) and Plus (currently never
), rather than Semigroup and Monoid. Since Monoid should probably concat the values within the promises, rather than racing.
We could still leave concat
for backcompat in 1.x.
Thoughts?
This is not complete, but wanted to open it for discussion. Inspired by @bergus' comment here, it was trivial to add fantasy-land 2.0 compat, while maintaining back compat with current creed 1.x.
If we want to go this route, then there's a bit more todo:
Merge #34, then update this PR to add namespaced bifunctor