ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Conor MacNeill" <co...@cortexebusiness.com.au>
Subject RE: [PATCH] Nested Tasks
Date Mon, 29 Jan 2001 05:41:45 GMT
Thomas,

I think we need to have a debate about whether nested tasks is a desirable
concept to implement. Whilst I can see how it might be desirable for your
approach to multithreading, it can open up a Pandora's box of complexity. I
don't think the debate last time was conclusive.

Anyway, ignoring those concerns, let me give you some feedback on your
patch. Firstly, please update to the latest version from CVS before doing
your diff. I am not sure how you have done it but the patches you have
submitted would undo a whole stack of recent changes.

> - A new Interface called SupportTasks : Used to mark
>    any task as a task supporting nested tasks

OK. I still don't like the name :-). I would go for something like
TaskContainer. Not a bug deal

> - A new Version of ProjectHelper.java :
>      - Constructor TaskHandler: Type of target changed to Object

I think that should be changed from Object to TaskContainer. It would better
not to use Object where we know it needs to be something else. Also the name
target should be changed in this case since it is confusing. There are other
uses of target in ProjectHelper which are also confusing because they refer
to tasks and not targets.

>      - NestedElementHanlder: Method init changed a lot

The usage

catch (BuildException bex) {
    if (target instanceof SupportTasks) {
        // Handle nested Tasks
        new TaskHandler(parentHandler, target).init(propType, attrs);
    } else {
         throw bex;
    }
}

seems wrong to me. You shouldn't be checking for non-exceptional cases in
the catch block, IMHO. Why not move this into the try block and handle it
there. Perhaps, a change in introspectionHelper would be needed to make that
work cleanly. In the end you are assuming that the cause of the build
exception is due to the inability to create a nested element, and that is
not a good thing.

> - A new Version of Target.java : Only added implements
>    SupportTasks and use the new method perform() from
>    Task (see below)

It is an interesting question about who should be responsible for firing the
task started and task finished events. Should it be the task or the
container. I actually don't have a strong opinion either way.


> - A new Version of Task.java :
>      - New method called perform() for a centralised handling
>        of task execution.
>      - Logging strategy; this enables task to redirect the
>        loggeroutput of there children (e.g. postpone, etc.)

I haven't looked into the logging strategy, yet.

Conor



Mime
View raw message