Skip to content

Conversation

bahamas10
Copy link
Contributor

this allows for a level of indentation to be dropped, which can be useful when trying to stay within 80 columns for source files that make complex use of vasync (looking at VM.js)

Before

vasync.parallel({
    'funcs': [
        function f1 (cb) { cb(); },
        function f2 (cb) { cb(); },
        function f3 (cb) { cb(); },
    ]
}, function (err, results) {
    console.log('done');
});

After

vasync.parallel([
    function f1 (cb) { cb(); },
    function f2 (cb) { cb(); },
    function f3 (cb) { cb(); },
], function (err, results) {
    console.log('done');
});

@davepacheco
Copy link
Contributor

I'm pretty torn about this. I feel I've wasted a lot of time on DWIM-style interfaces, both debugging callers and just reading code written with them. Because this change looks only at the argument's type (not presence), it's simpler than a lot of others, but there's a slippery slope. We've generally advocated that non-trivial public interfaces take named arguments (the way the interface does now). It's not as though we don't expect to have more options here: there's already maxConcurrency (see #27) and trackTiming (which I've been working on separately) in the works.

I've wanted this sugar in the past, and for the same reason. But the more I think about it, especially given that once #27 is implemented I think we'll want to discourage use of parallel() without maxConcurrency, I don't think this change is worthwhile. I'd be interested in other people's thoughts here.

@trentm
Copy link
Contributor

trentm commented Jan 5, 2016

Similarly torn. FWIW, to save a level of indentation I typically do this:

vasync.parallel({funcs: [
    function f1 (cb) { cb(); },
    function f2 (cb) { cb(); },
    function f3 (cb) { cb(); },
]}, function (err, results) {
    console.log('done');
});

@davepacheco
Copy link
Contributor

Obviously I've been conflicted -- I filed #19. Sorry for the waffling.

@bahamas10
Copy link
Contributor Author

@trentm interesting - that should help out a bit with this new vminfod coming (lot's of parallel jobs spawning series jobs, etc.)

@davepacheco Yeah, I too see both sides of this and I really don't know what is the best approach here. Like you said this implementation is not terribly egregious as it only checks Array.isArray(), not something like arguments.length... but just because an implementation is relative un-dirty does not make it the best approach.

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.

3 participants