fixed async doWhilst

[murcal]

No description provided.

[dt-bot]

async/async.d.ts

to authors (@borisyankov @Kern0 @Penryn). Could you review this PR?
👍 or 👎?

Checklist

• pass the Travis CI test?

[joe-herman]

Thanks for the contribution, but unfortunately this doesn't follow the API outlined at https://github.com/caolan/async#doWhilst

👎

[DominicBoettger]

Yes, but i think the API makes no sense at all, cause writing a test function without a parameter or a dataset which is testable is not really a test function ;-).

[joe-herman]

You're welcome to open a ticket at caolan/async. Typescript definitions must exactly match a module's API regardless of your feelings for or against it.

[DominicBoettger]

The API documentation is wrong and it's not about feelings. It's just working and correct. I already opened a ticket.

[vvakame]

note caolan/async#1107
https://github.com/caolan/async/blob/master/lib/doWhilst.js

[joe-herman]

Ok so looking at the changelog for this PR we had

 doWhilst(fn: AsyncVoidFunction, test: () => boolean, callback: (err: any) => void): void;

Now replaced by

 doWhilst(fn: (Function), test: (data: any) => boolean, callback: (err: any) => void): void;

Looking at https://github.com/caolan/async/blob/master/lib/doWhilst.js#L29 I still don't think this is right.

It seems @DominicBoettger you are right about that particular case (this is basically undocumented behavior). However, the existing repo isn't wrong (ie: no breaking changes), and these commits aren't right.

Ideally, AsyncVoidFunction should not have been changed to Function because it is less specific, and test: (data: any) is still not correct because the API does not require data. It does pass other arguments onwards, but these commits don't reflect that. This PR is a good effort and highlights the problem, but is just a bit incorrect (sorry @murcal!).

I still vote 👎, and welcome anyone else to weigh in!

Perhaps a new PR should be opened to change test: () => boolean to test: (...arguments: any[]) => boolean or similar, because that's what the internal API does. I can do this myself at some point if no one beats me to it.

[RyanCavanaugh]

Hello and thank you for your contribution!

Due to an excessively long queue of pull requests that have become stale, we are "declaring bankruptcy" and closing all PRs opened before May 1, 2016. If you'd still like to merge this code in, please open a new PR that has been merged and rebased with the master branch.

Going forward, we are committing to review or merge all PRs on a regular basis so this bankruptcy will not occur again. We apologize for the incovenience and hope you will continue to contribute to DefinitelyTyped in the future.

Thanks
The TypeScript and DefinitelyTyped teams