On 04/09/2011, at 12:47 AM, Adam Murdoch wrote:
> On 03/09/2011, at 3:07 AM, Luke Daley wrote:
>
>> Hi all,
>>
>> I just realised that we have a slight problem with our domain collections.
>>
>> Consider the assemble task:
>>
>> private Task addAssemble(Project project) {
>> Task assembleTask = project.tasks.add(ASSEMBLE_TASK_NAME);
>> assembleTask.description = "Assembles all Jar, War, Zip, and Tar
>> archives.";
>> assembleTask.group = BUILD_GROUP
>> assembleTask.dependsOn
>> project.tasks.withType(AbstractArchiveTask.class)
>> }
>>
>> We are interested in the last line; It wires a filtered domain collection to
>> the dependsOn of the assemble task.
>>
>> Users are going to be tempted to write this:
>>
>> assemble.dependsOn.remove(someJar)
>>
>> This is going to explode because we don't support any mutation options on
>> filtered domain object collections, forcing the user to write:
>>
>> assemble.dependsOn = assemble.dependsOn - [someJar]
>>
>> A more concerning problem than the verbosity is the fact that we've lost
>> lazy wiring. Any new archive tasks are not going to be wired in as
>> dependencies.
>>
>> Two possible solutions (I can see):
>>
>> #1. Enhance our FilteredDomainObjectCollection (and friends) to support
>> mutation (more correct solution, but not trivial to implement properly).
>> #2. Never assign filtered collections, but use events to manipulate (example
>> follows)
>
> Some other solutions (not necessarily mutually exclusive):
>
> #3. Use a better default, so that removal needs to happen less often. For
> example, I think it would be much better if 'assemble' built the publications
> of the project, rather than all the archives of the project. ie match the
> implementation more closely to the intent of the 'assemble' task, which is to
> produce the outputs of the project.
This sounds right for this context, but I think this is going to pop up
elsewhere.
> #4. Similar to #1, but as a decoration over the domain object collection.
> There are quite a few places where we have a collection which is a composite
> and lazily composed from objects of many different types. Tasks.dependsOn is
> one such place. You can add strings, Tasks, iterables, arrays, closures,
> callables, and so on to it, and the resulting set of tasks are built by
> flattening and resolving all these objects. Project.files() and
> ConfigurableFileCollection, and SourceTask are more examples of this approach.
>
> This approach would involve collecting up any removed values as unresolved
> values in a separate collection. When we resolve the collection's contents,
> we resolve the added value, then resolve the removed values, and return the
> difference of the two. What this means is that you can do this kind of thing:
>
> assemble.dependsOn.remove('jar') // excludes 'jar' task, regardless of how it
> was added
> assemble.dependsOn.remove(tasks.withType(Zip)) // excludes all Zip tasks,
> regardless of how they were added.
>
> It might be good to introduce a Collection/Set subtype which represents these
> kinds of lazily resolved collections, where you manipulate a collection of
> unresolved values, which are later resolved into the actual values.
>
> Some benefits of this approach:
> * We can extract a shared base class (or a shared strategy class, or
> whatever) that represents how we flatten various types: collections,
> iterables, arrays, closures, etc). At the moment, this is spread a little
> through various places, and is, of course, inconsistent.
> * We can reuse the implementations for solving our 'collection convention
> mapping' problem, which is really just the same lazy resolve problem as
> above, except that the raw types are the same as the resolved types, ie with
> a different resolution strategy.
> * We have a place to add methods where you can mess with the resolving
> process: eg
>
> tasks.dependsOn.exclude { it.name.startsWith('prefix-') }
I am *strongly* for this. We should record this as something to do.
>> project.tasks.withType(AbstractArchiveTask.class).all {
>> assembleTask.dependsOn.add it
>> }
>> project.tasks.withType(AbstractArchiveTask.class).whenObjectRemoved {
>> assembleTask.dependsOn.remove it
>> }
>>
>> There is a serious problem with #2 though in that it becomes impossible for
>> the user to undo the auto wiring. I don't think this is a viable solution.
>>
>>
>> Actually… it's just occurred to me that there is a (relatively) easy to
>> implement solution for #1.
>>
>> For add operations, we could create a new composite collection of the
>> filtered collection plus the addition. For removal operations we could
>> simply derive a new filtered collection with a filter that matches anything
>> but what is to be removed. This would definitely work, but it seems like it
>> could lead to an object explosion and slow iteration in extreme cases.
>
> This isn't quite right, I think. DomainObjectCollection.withType() and
> matching() should work like List.subList(), so that they are live, where
> mutations made through the filtered collection are visible in the backing
> collection.
I think you can make the argument either way depending on the context, i.e.
it's awkward to meet the JDK Collection requirements either way. I think the
best thing to do would be to not allow mutation of filtered collections and
implement your point 4.
--
Luke Daley
Principal Engineer, Gradleware
http://gradleware.com
