gulpjs / undertaker

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

Should tasks inherit the `branch` flag #51

Closed phated closed 8 years ago

phated commented 8 years ago

@erikkemperman I am taking a look at implementing https://github.com/gulpjs/gulp/issues/1411#issuecomment-161072194 and https://github.com/gulpjs/gulp/issues/1330 through the use of the branch flag you added to the metadata (awesome idea, btw). However, when we do something like the following, that flag isn't attached to the metadata for the new task.

gulp.task('default', gulp.series(...));

Do you think that the task should inherit the branch flag if we already have metadata and the metadata tells us it is a branch?

phated commented 8 years ago

Implementation at https://github.com/gulpjs/undertaker/commit/3536ee71555cd2ca2c0bae598b8c029de6d5fb17 - needs tests

erikkemperman commented 8 years ago

@phated So, the idea is to use the branch property to make decisions about error propagation and log level, have I got that right?

I added the flag to be selective about when to do nodes.push in set-task.js -- before it'd always do that if metadata already existed for the fn at hand, the assumption apparently being that this would only happen for series/parallel constructions, which was no longer true with aliases.

I don't see how it would functionally wrong to have tasks "inherit" this property from their function (although I might not call it that)... But I didn't have a lot of time playing around with it.

With the gulp-cli branch branches-debug-log, which I assume is the intended corresponding change to passing the branch flag along in createExtensions, the output hides too much in my opinion. In stead of just suppressing <series> and <parallel> lines it also suppresses the parent task.

Maybe it's just me but if I run gulp foo I expect to see Starting foo and Finished foo in the output, without having to do -LLLL. So I guess that might be an argument against inheriting the branch property in this way.

I'm not too familiar with error propagation, but wouldn't it be pretty easy to just mark an error as having been logged, and not printing it again in that case? That way you could separate error propagation from error logging.

phated commented 8 years ago

@erikkemperman Yeah, I agree that running gulp foo should show "Starting foo" and "Finished foo" but if the branch flag doesn't transfer to the named task, some error logging doesn't get hidden (errors from series/parallel are hidden) with this change. Maybe that's okay. I don't know of a good way to handle the error propagation

erikkemperman commented 8 years ago

@phated Not at a computer atm, but just a thought: maybe pass along meta.tree.type as well? I.e., the combination of branch and type might allow you to print or suppress in the appropriate circumstances?

erikkemperman commented 8 years ago

Or, now looking at your gulp-cli change, maybe only use the branch property for logging errors, leaving the start/finish stuff same as before..

phated commented 8 years ago

@erikkemperman I'm going to close this. I've come up with a better way to handle the error propagation and I'm only going to filter the <series> and <parallel> logging based on the branch flag.

erikkemperman commented 8 years ago

@phated sounds good to me!