gulpjs / undertaker

Task registry that allows composition through series/parallel methods.
MIT License
200 stars 31 forks source link

Modify for gulp --tasks #53

Closed sttk closed 8 years ago

sttk commented 8 years ago

I tried to achieve @tylersticka's second image in gulpjs/gulp#1294 and finished.

sttk commented 8 years ago

Sorry for some errors but I fixed them all.

phated commented 8 years ago

Many of the changes in here aren't required fir the CLI changes, including the description changes and the task function changes. The transformer code is interesting but very complex and possibly not needed.

phated commented 8 years ago

To clarify, the undertaker.task() method is a setter and a getter for tasks, it should not return a task when it is being set.

How users should set description:

function clean(cb){ cb() }
clean.description = 'remove some folders';
gulp.task(clean);
sttk commented 8 years ago

The transformer code is interesting but very complex and possibly not needed.

I got it. I'll try to remove the transformer from undertaker#tree and not use it in gulp-cli#logTasks.

sttk commented 8 years ago

How users should set description:

Isn't there a case like the following?

function clean(cb) { cb() }
clean.description = 'remove some folders';
gulp.task(clean);
gulp.task('another task name', clean).description = 'another description';

Or don't you want to write like the following?

gulp.task('clean', function(cb) { cb() })
 .description = 'remove some folders';
sttk commented 8 years ago

How about the depth flag and the sorting of undertaker#tree. Should the sorting be optional?

phated commented 8 years ago

I'd prefer not to allow chaining description because that requires the setter to return the function. If you are able to reduce the transformer code, I'm good with leaving it in here. Not sure about the sorting but you can use lodash sortBy for it

phated commented 8 years ago

I'd put depth in the options, I think.

sttk commented 8 years ago

I've finished modifying the program!

I'd prefer not to allow chaining description because that requires the setter to return the function.

OK. I removed the chaining description. However it's not difficult to implement it again, so I may do so if you or anyone want it later.

Not sure about the sorting but you can use lodash sortBy for it

Yes, I used it in my code. It is easy to write as needed, so I removed the sorting from undertaker#tree not to change the original behavior preferably.

sttk commented 8 years ago

Thank you for detail reviews.

I want to move all changes to gulp-cli and make undertaker no change.

Because, though my first plan contains chaining description and control of depth in tree function, we removed the former and the latter can be moved to gulp-cli, so it isn't necessary to modify undertaker any more.

I'll implement what you pointed out here into gulp-cli.

About removing opts.deep I think it is better to separate from this PR.

Can I close this PR about undertaker with no change?

phated commented 8 years ago

Sounds good!