Add fast failing to all remaining curried functions #69
Conversation
|
|
||
| function mapRej$mapper(mapper, m){ | ||
| if(!isFuture(m)) invalidArgument('Future.mapRej', 1, 'be a Future', m); | ||
| return new FutureMapRej(m, mapper); |
There was a problem hiding this comment.
In #66, this line incorrectly contained return new FutureMapRej(mapper, m);
fluture.js
Outdated
|
|
||
| function fold$f$g(f, g, m){ | ||
| if(!isFuture(m)) invalidArgument('Future.fold', 2, 'be a Future', m); | ||
| return new FutureFold(f, g, m); |
There was a problem hiding this comment.
This line is also incorrect. The first argument should be m.
fluture.js
Outdated
|
|
||
| function finally$left(left, right){ | ||
| if(!isFuture(right)) invalidArgument('Future.finally', 1, 'be a Future', right); | ||
| return new FutureFinally(left, right); |
There was a problem hiding this comment.
These arguments were also flipped compared to the previous version, which was a far less obvious breaking change.
| @@ -2,113 +2,65 @@ | |||
|
|
|||
| const expect = require('chai').expect; | |||
| const Future = require('../fluture.js'); | |||
| const FutureAnd = Future.classes.FutureAnd; | |||
There was a problem hiding this comment.
I prefer this new way of testing, because it tests behavior rather than implementation.
| }); | ||
|
|
||
| //TODO: Argument order. | ||
| testInstance((a, b) => Future.and(a, b)); |
There was a problem hiding this comment.
Future.and was one of the functions I implemented without a createDispatcher from the start, and I made the same mistake there that caused all this mess now. Essentially the argument order of Future.and is the wrong way around (static functions should be flipped). If I'd fix that now, it would be a breaking change, hence the #TODO.
Codecov Report
@@ Coverage Diff @@
## master #69 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 831 889 +58
Branches 182 214 +32
=====================================
+ Hits 831 889 +58
Continue to review full report at Codecov.
|
test/5.future-both.test.js
Outdated
| }); | ||
|
|
||
| //TODO: Argument order. | ||
| testInstance((a, b) => Future.both(a, b)); |
There was a problem hiding this comment.
See my comment on future-and.test.js.
There was a problem hiding this comment.
Actually, it's quite expectable that Future.both(ma, mb) would give me Future [a ,b] and not Future [b, a]. I'm going to remove this comment.
|
Functions that turned out to be broken after fixing the tests:
It's surprising nobody talked to me about experienced issues over the nine days that this was live. |
Finally finally closes #4
Avaq
requested review from
davidchambers
and removed request for
davidchambers
Breaking changes - #74 The argument order of `Future.or()` has been flipped - #75 `Future.isForkable()` has been removed - #75 `Future.fromForkable()` has been removed - #75 `Future.cast()` has been removed New features - #70 Add a new ConcurrentFuture type Bug fixes and improvements - #4 #69 All curried functions now fail fast - #59 Update Sanctuary interoperability - #73 `Future.finally()` now runs `finally` computation when cancelled
I've reverted the commits involved in #66 because it broke multiple of the affected functions. The reason some of the functions broke is because of arguments being passed into class constructors in the wrong order. The reason this wasn't caught by unit-tests is because all I'm asserting for the static constructors, is that they return the expected class instance. The actual test of the class instances themselves use instances created by methods (rather than the static functions affected in this commit).
Now I'm reluctant to merge this until I know that these kind of mistakes would be caught by the unit tests. Maybe I should run all instance tests on several instances, each created in a different way. For now that means that most instance tests would run twice.