Closed satazor closed 11 years ago
Hey André, thanks for the feedback!
config
as configuring the task as a whole. I can agree with getting rid of the options
inside config, as that is a holdover from grunt's current configuration format. However, the spec needs to state that files
is a reserved key which must hold a valid file listing as shown in the examples in the README. Assuming we do this, the only difference we have is that you want to call config
options
, yes?parseConfig
method provides a clear separation of responsibilities in the lifecycle of a task, and it makes testing easy. Additionally, because node-task is written in a functional style, processing options in setup would require the setup method to returned the parsed config, which feels wrong to me.run
method, which must return a value or a promise if the execution is async. It is up to the task runner to decide if it waits for the promise to resolve before kicking off another task, not the task itself.method
is only used in my implementation of the task generator. Anything you see suffixed with ⊄ only applies to that. Now that I have a working prototype, I should probably split the spec and the implementation into two separate repos to prevent this confusion.I view config as configuring the task as a whole. I can agree with getting rid of the options inside config, as that is a holdover from grunt's current configuration format. However, the spec needs to state that files is a reserved key which must hold a valid file listing as shown in the examples in the README. Assuming we do this, the only difference we have is that you want to call config options, yes?
Yep
The parseConfig method provides a clear separation of responsibilities in the lifecycle of a task, and it makes testing easy. Additionally, because node-task is written in a functional style, processing options in setup would require the setup method to returned the parsed config, which feels wrong to me.
The more things the spec has, the more confusing users get. I think that, besides testing, we could merge both into only setup
. The setup does not need to return the options
if users would be manipulating the options passed. Other way around, which I think it's better, is if the spec actually provide a way to define the task optons. Then, task runners will guarantee to fill in the default options into the initial options object. This way, users will rarely need to implement the setup method unless they want to modify or infer new ones (therefor the parseConfig could be deleted). This would also be good for task runners to present options in the task usages (cli).
I'm not sure I follow what you mean when you say you don't like how tasks are declared to be async. There is no declaration that a task is async. The only interface between task runners and tasks is the run method, which must return a value or a promise if the execution is async. It is up to the task runner to decide if it waits for the promise to resolve before kicking off another task, not the task itself.
What I'm saying is that we should follow node conventions. Passing callbacks in node is more popular than promises. Promises have advantages, but in this case they aren't needed (imo). Having in mind the task runners only know about the run
method, I would make the run
signature like this run: function (opts, next)
always. For the task runner, the run is always async, but could be in fact sync.
The spec doesn't state how files should be read or written, it only specifies the method names which should be used when doing so.
So the actual spec defines things like readFile
and writeFile
. I'm not sure if those file
only attributes should be part of the (base) spec. I see them a particular case of the spec, hence the suggestion about an extended spec.
parseConfig
instead of the task itself. I'm probably missing something, though. Can you provide a working example of what you mean? Also, I am strongly opposed to mutating the state of the incoming config. This spec is written in a functional style.Boiling awful code like this: https://github.com/gruntjs/grunt-contrib-less/blob/master/tasks/less.js
...into something like this:
var Task = require('task');
var less = require('less');
var task = Task.create({
name: 'less',
description: 'example for less compilation',
filterRead: function (config, input, filepath) {
var defer = Task.defer(); // this is just a proxy to when.defer();
var parser = new less.Parser(config.parse);
parser.parse(input, function(err, tree) {
defer = tree.toCSS(config.render);
});
return defer;
}
});
Is a huge win for task writers and the maintainers of task runners. If an exception happens anywhere in this code it bubbles up to task.exception
.
type
property with valid options of fileReader
/ fileWriter
/ fileIterator
is not a part of the spec at the moment but the more I think about it, the more I think it wouldn't hurt if it was.Also, with regards to promises, my implementation of node-task makes it so task writers can return a value or a promise from literally any method defined in the spec and the run
method will just work, resolving correctly or throwing an exception if needed.
This stands in stark contrast to passing callbacks, which would make filterRead
and filterWrite
(the primary interface most task-writers will use to process files) considerably more complex and error prone.
The practice of passing callbacks generates a sea of accidental complexity that has nothing to do with the problem you are trying to solve.
I've now understood the spec, and I was confusing the spec with the task implementation itself. I will start a new topic soon.
:+1: on promises
Braindump after reading the spec. I will only point out the things that feel rather strange or I disagree:
config
that hasoptions
inside instead of onlyoptions
? There is an example in which the files are part of theconfig
and not theoptions
. I disagree. Files should be part of theoptions
. What else is part of theconfig
?parseconfig
when users could parse their options in thesetup
?mocha
, detecting the callback as the last argument (if present, then it is async, otherwise it is sync). Please note that everything should have this behavior and not only the main method itself. Thesetup
,teardown
, etc should also be possible to run in an async way. Also, making users to usedefer
is strange. The standard way of doing it in nodecallback(error)
works great.method
is a good name, we should find a better one.spec
should not worry about this. Maybe an extension of thisspec
? What's your thoughts on this?