Implement task parameterization #281
Conversation
| @@ -0,0 +1,267 @@ | |||
| // Copyright 2015 Google. All rights reserved. Use of this source code is | |||
There was a problem hiding this comment.
Some of the methods in this file are not yet directly unit tested. That's because they are mostly copied and pasted from unscripted with slight modifications, and so I want to factor them out into a separate library or package that can be shared by unscripted and grinder. Plan to do that as part of implementing the annotation based API.
|
It is a doozy - sorry for the delay in taking a look at it.
This does make the PR harder to review - it's not easy to tell at a glance which changes are semantic ones... That aside, given the size of the PR, the source code re-org, and the fact that it has some smaller breaking changes, I think we should push out a last minor release before this lands, and then plan on doing a full major rev. It'll capture this PR, give you some time to do the additional changes mentioned, and let us remove the 0.7 deprecated code. |
| addTask(new GrinderTask( | ||
| 'bump', | ||
| taskFunction: () { | ||
| var releaseType = context.invocation.positionals.first; |
There was a problem hiding this comment.
Can we make accessing the args more terse, or add aliases to help references to the args be terse?
context.args.rest.first;
context.args.first;
context.args['foo];
args.rest.first;
args['foo'];
thoughts? This: context.invocation.positionals.first strikes me as pretty long, given that it may be the most common use of task args.
There was a problem hiding this comment.
I don't anticipate many folks actually needing to use the imperative API introduced in this PR, once I add the follow up annotation-based API which will look something like:
@Task('bump the version')
bump(
@Positional(help: 'Which type of release to do', allowed: ['release', 'build', 'patch', 'minor', 'major'])
String releaseType,
{bool pre: false
String preId}) {
// ...
}which is about as terse as it could possibly get.
The name invocation is to match TaskInvocation. I considered TaskArgs, which would match up with args, but that doesn't really capture the fact that it contains the task name as well. And "invocation" is already a familiar concept for anyone who has used dart:mirrors.
There was a problem hiding this comment.
Ah, ok, cool. The 95% case will be the annotations API, which will be nice and terse; makes sense.
Yep, totally my fault.
Sounds good, so first #278 then merge this, then #138, then the follow up issues I mentioned above, then release 0.8? |
|
OK, lgtm! |
Implement task parameterization
@devoncarew
This allows defining parameters for tasks, and accepting arguments for them when invoking tasks either from the command-line or when used as dependencies.
It doesn't yet include:
Option,Flag,Positional, andRest(hesitant to do this).Restargument (multiple homogeneous args). Main reason is the same as it is for unscripted, which is that dart doesn't support having both named and positional optional parameters in the same signature, so they couldn't be defined through the forthcoming annotation based API. There may be a way to add it in the imperative API though.I will file separate issues for each of these, once this one lands.
This PR also splits out most of
lib/grinder.dartinto smaller libraries underlib/src. I didn't intend on doing that in this PR, but I added a couple new libraries, and wanted to use just one small part oflib/grinder.dartso factored that part out, but then that depended on another small part, so had to factor that out as well, and it snowballed from there.