FIX: Reconstruct multiple fragments #41
Conversation
lamphamsy
changed the title
node-eclib.js
Outdated
| this.reconstructFragment(availFragments, id, | ||
| function recon(err, fragment) { | ||
| if (err) { | ||
| callback(err); | ||
| return callback(err); |
There was a problem hiding this comment.
You cannot return early from a forEach and using this would call the callback multiple times. You would have to use a vanilla for loop or use some array method like arr.every
There was a problem hiding this comment.
thank you, it's my bad :(
node-eclib.js
Outdated
|
|
||
| // Recover all missing fragments one by one. | ||
| fragmentIds.forEach(function reconstructEach(id) { | ||
| fragmentIds.forEach(function reconstructEach(id, index) { |
There was a problem hiding this comment.
Additionally, if you use an arrow function here you don't have to bind this for context.
There was a problem hiding this comment.
Tests are still running on node 0.10 (as can be seen with the CI) since the aim of this library was extensive compatibility. That means no arrow functions. We can probably jump our lowest threshold, but that would be a breaking change.
There was a problem hiding this comment.
Oh interesting. I didn't realize that this project was using node 0.10.
Fragments reconstruction runs in parallel. Hence recovered fragments could be not in right order. Fix sort function
lamphamsy
force-pushed
the
fix/repair_multifrags
branch
from
0ea160b
to
0b699cf
Compare
|
A reinforced functional test for reconstructing multiple fragments is updated: #43 |
|
Merge to working branch |
Fragments reconstruction runs in parallel. Hence recovered fragments
could be not in right order.
Fix sort function