ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From stephan beal <step...@wanderinghorse.net>
Subject Re: PATCH: cvs <commandline> implementation
Date Mon, 25 Mar 2002 07:42:45 GMT
On Monday 25 March 2002 06:36 am, Erik Hatcher wrote:
...
> CDATA
...

i appear to be the only one who'll use this ability, so i'll just keep 
re-patching my local copy for this capability. i FAR prefer it over the 
<commandline> approach, for reasons of brevity.


> You have to 'if' statements with identical conditions.  Can't you collapse
> this some?

Those are to implement legacy behaviour correctly while also making 
command='' optional if another form of command is set. Note how setCommand() 
is called in the first one.

> +    public String getText() {
> +        return this.cdata;
> +    }
>
> Why do you need this?  Maybe I missed where its referenced, but we
> typically don't have getters.  No big deal though.

Because i design for subclassibility, and had problems subclassing the Cvs 
task in 1.4.1 because it was missing getters. i had to override setters just 
to get at some variables i needed, and found that to be very annoying. Even 
if the data members had been protected, i would have provided a getter 
because i feel that protected data (as opposed to functions) is evil (as do 
the Ant design guidelines ;).


> +    public void addConfiguredCommandline( Commandline c ) {
> +        this.addConfiguredCommandline( c, false );
> +    }
>
> Why exactly do you need addConfiguredCommandline rather than just
> addCommandline?

For the consistent handling of command='' and <commandline> elements using 
the same code. It may be doable with just addCommandline(), but this 
sollution seems the most extendable to me.


> +    /**
> +    * Configures and adds the given Commandline.
> +    * @param insertAtStart If true, c is
> +    */
> +    public void addConfiguredCommandline( Commandline c, boolean
> insertAtStart ) {
>
> Why is this public?  I'd recommend private or protected.

Fair enough. i would suggest protected, but that's because i like subclasses 
;).


> Index: src/main/org/apache/tools/ant/types/Commandline.java
> ===================================================================
> I'm uncomfortable with the changes made to Commandline, simply because its
> used by other code and we need to be careful these changes don't break
> anything.   

It's backwards compatible.


> Why not just set the arguments in the order you want them
> rather than having a positional insert option?  

Because that requires duplicate code in AbstractCvsTask because of the way 
command='' and <commandline> work.

> I would have to spend a
> fair bit of time analyzing it in more detail to make sure I didn't spot any
> issues - I just don't have that kind of time right now.  Anyway you keep
> your changes to just the CVS task code and not touch this module?  And you
> did mention breaking Javac while you were at it - making my uneasiness
> factor greater!

But i fixed it, and that's why i'm confident that it doesn't break existing 
code. ;) Had it NOT broken anything i would be nervous, but after recovering 
from having broken <javac>, i'm not converned.

Thanks for the comments!

----- stephan
Generic Unix Computer Guy
stephan@einsurance.de - http://www.einsurance.de
Office: +49 (89)  552 92 862 Handy:  +49 (179) 211 97 67
"...control is a degree of inhibition, and a system which is perfectly
inhibited is completely frozen." -- Alan W. Watts

--
To unsubscribe, e-mail:   <mailto:ant-dev-unsubscribe@jakarta.apache.org>
For additional commands, e-mail: <mailto:ant-dev-help@jakarta.apache.org>


Mime
View raw message