Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make async.transform actually support 2 arguments #1381

Merged
merged 3 commits into from Mar 14, 2017

Conversation

webbiesdk
Copy link

The documentation suggests that both the accumulator and callback arguments to transform is optional.
(https://caolan.github.io/async/docs.html#transform)

However, if both are left out (and transform therefore is called with only 2 arguments), transform crashes with a iteratee is not a function exception.

By changing the equality check to a less than check, the transform function support the use-case where only 2 arguments are passed.

The documentation suggests that both the `accumulator` and `callback` arguments to transform is optional. 

However, if both are left out (and transform therefore is called with only 2 arguments), transform crashes with a `iteratee is not a function` exception. 
This fixes that.
@webbiesdk
Copy link
Author

I have no idea why the two builds are failing.

The coverage test claims that some internal filter.js has less coverage. Even though my change doesn't affect that.

And somehow a test-case failed, after I added a test-case in another file.

@webbiesdk webbiesdk closed this Mar 13, 2017
@webbiesdk webbiesdk reopened this Mar 13, 2017
@webbiesdk
Copy link
Author

I just triggered a rebuild by closing and re-opening the issue, and now the tests are passing.

You guys have an unstable test!

@megawac
Copy link
Collaborator

megawac commented Mar 13, 2017

Yeah, its a reoccurring issue (#1322) sorry about that.

As for the PR, sure transform could support taking a source to iterate and an iteratee method without a final callback but what would be the point?

@megawac
Copy link
Collaborator

megawac commented Mar 13, 2017

Also the test failing looks to be coveralls complaining that the coverage of the repo decreased for some reason

@webbiesdk
Copy link
Author

As for the PR, sure transform could support taking a source to iterate and an iteratee method without a final callback but what would be the point?

The point would be that your current documentation states that callback is optional.
The current implementation actually supports an optional callback, but only if the number of arguments is exactly 3.

@webbiesdk
Copy link
Author

webbiesdk commented Mar 14, 2017

Also the test failing looks to be coveralls complaining that the coverage of the repo decreased for some reason

The test that failed until i triggered the rebuild was: retry with interval when some attempts fail and error test returns false at some invokation (but it is already marked as flaky in the test-source).

I also tried to add a new test to cover a bit of the uncovered code, and now the coverage result makes no sense, since it states that every file with changed coverage has the same or increased coverage (but somehow the overall coverage is still down).

I also tried to run npm run coverage with and without my change, and it reports the exact same coverage in both cases.

The decreased coverage is caused by a change in the environment.
To confirm that i looked at the last coverage report that gave high coverage. Just try to look at these two:

https://coveralls.io/builds/10391324/source?filename=lib%2Finternal%2FsetImmediate.js (the last time coverage was run on master)
https://coveralls.io/builds/10579695/source?filename=lib%2Finternal%2FsetImmediate.js (coverage test after my change)

@megawac
Copy link
Collaborator

megawac commented Mar 14, 2017

Thanks, yeah the retry test failing is a known issue. Thanks for pointing out the docs were inaccurate.

I'll look into this coverall issue issue later, seems odd its complaining when you've added tests.

Thanks for the pull request!

@megawac megawac merged commit 3d79745 into caolan:master Mar 14, 2017
@webbiesdk webbiesdk deleted the patch-1 branch March 14, 2017 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants