ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Erik Hatcher" <jakarta-...@ehatchersolutions.com>
Subject Re: PATCH: cvs <commandline> implementation
Date Mon, 25 Mar 2002 05:36:32 GMT
Stephan,

I'll comment in-line to your patch....

----- Original Message -----
From: "stephan beal" <stephan@wanderinghorse.net>

Index: src/main/org/apache/tools/ant/taskdefs/AbstractCvsTask.java

+    /**
+     * see addText()
+     */
+ private String cdata = null;

I'm personally not too hip on the CDATA section given the flexibility of the
<commandline> capabilities - and if the only reason to keep it is to make
your Perl code a little cleaner, then its hard to be for it, since its
applicability is only for you.  Others have thoughts on this?

+    /** (It just bugs me to leave this to the compiler.) */
+    public AbstractCvsTask() {
+    }
+

I'd recommend refraining from making too many "silly" comments - not really
something we want to commit to the Ant codebase.

+    /**
+This reads in any data set via addText() and treats each line as one cvs
command,
+executing it via runCommand().
+    */
+    protected synchronized void processCData() throws BuildException {
+        if( this.cdata == null || this.cdata.length() < 1 ) return;
+        java.util.StringTokenizer st = new java.util.StringTokenizer(
this.cdata, org.apache.tools.ant.util.StringUtils.LINE_SEP );
+        String mycmd = null;
+        try {
+            while( st.hasMoreElements() )
+            {
+                mycmd = ((String)st.nextElement()).trim();
+                if( mycmd.length() == 0 ) continue;
+                if( mycmd.startsWith( ";" ) ) { continue; }
+                if( mycmd.startsWith( "#" ) ) { continue; }
+                mycmd = getProject().replaceProperties(mycmd);
+                Commandline c = new Commandline();
+                this.configureCommandline( c );
+                c.createArgument().setLine( mycmd );
+                //log( "processCData() cvs command: ["+c.toString()+"]",
Project.MSG_DEBUG );
+                //log( "processCData() cvs command: ["+c.toString()+"]" );
+                //this.setCommand( mycmd );
+                this.runCommand( c );
+            }
+        }
+        catch( BuildException e ) {
+            throw( e );
+        }
+        catch( Exception e ) {
+            throw new BuildException( e, location );
+        }
+    }

See comments above about CDATA.  Just seems unnecessary, and the benefit to
not having it is that the above code could be axed.  Less code is better!
:)

+        String warning = "ACHTUNG, BABY: proceeding only by the grace of
failonerror='false': ";

Again, this is not something I'd want in Ant's codebase, so best to keep it
professional or risk committing delays or vetos! :)

+    public void execute() throws BuildException {
+
+
+        if( this.getCommand() == null
+            && this.getText() == null
+            && vecCommandlines.size() == 0 ) {
+            // re-implement legacy behaviour:
+            this.setCommand( AbstractCvsTask.default_command );
+        }
+
+        String c = this.getCommand();
+        if( c != null ) {
+            this.addConfiguredCommandline( this.cmd, true );
+            this.cmd.createArgument().setLine(c);
+        }
+
+        // this isn't at the top of the function so we can re-implement
legacy behaviour of 'checkout'
+        if( this.getCommand() == null
+            && this.getText() == null
+            && vecCommandlines.size() == 0 ) {
+            throw new BuildException( "You must specify either the command
property, <commandline> elements, or character data (one cvs command per
line)!", location );
+        }

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

+    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.

+    public void addConfiguredCommandline( Commandline c ) {
+        this.addConfiguredCommandline( c, false );
+    }

Why exactly do you need addConfiguredCommandline rather than just
addCommandline?

+    /**
+    * 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.


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.   Why not just set the arguments in the order you want them rather
than having a positional insert option?  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!
:)

    Erik



--
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