[collections] TransformedCollection

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[collections] TransformedCollection

Rob Oxspring
Hi,

I've just been burnt (and not for the first time) by the fact that the
TransformedCollection doesn't transform the objects that are already in
the wrapped collection.  For example:

Collection dates = ... // a collection with Date objects in it
Transformer toString = ... // a transformer that calls toString
Collection strings =
     CollectionUtils.transformedCollection(dates,toString);

My intuition tells me that strings should be full of String objects and
have no Date objects in it but the opposite is true.  Is this the
intended behaviour or is this a bug?  I'm happy to patch the
TransformedCollection class but wanted to run it passed people first...

Thanks,

Rob

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [collections] TransformedCollection

Stephen Colebourne
The problem is that the collections you pass in (dates) is supposed to
be decorated. In order to transform the entries on construction there
would need to be some way to alter the values already in dates, but the
Collection interface provides no such direct mechanism.

However, it could be done indirectly:
a) Copy input collection to a list
b) Clear input collection
c) Transform each element from the list back into the original collection

If you would like to provide a patch (or just change SVN) to add a new
static method (decorateTransform?) for doing this on
TransformedCollection then please go ahead. (A new method is needed as
the javadoc on the existing decorate method is explicit)

Note, that if you do do this then the new method should be added to all
the Transformed* classes.

Stephen


Rob Oxspring wrote:

> Hi,
>
> I've just been burnt (and not for the first time) by the fact that the
> TransformedCollection doesn't transform the objects that are already in
> the wrapped collection.  For example:
>
> Collection dates = ... // a collection with Date objects in it
> Transformer toString = ... // a transformer that calls toString
> Collection strings =
>     CollectionUtils.transformedCollection(dates,toString);
>
> My intuition tells me that strings should be full of String objects and
> have no Date objects in it but the opposite is true.  Is this the
> intended behaviour or is this a bug?  I'm happy to patch the
> TransformedCollection class but wanted to run it passed people first...
>
> Thanks,
>
> Rob
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [collections] TransformedCollection

James Carman
In reply to this post by Rob Oxspring
Are you wanting to add dates to the resulting collection and have them
transformed into Strings?  Or, are you just looking for a collection of
Strings that you don't need to modify which represent the dates?  If it's
the latter, you should use the CollectionUtils.collect() method rather than
the transformedCollection() method.  I too have been burned by that very
thing.  Just by looking at the name, it would seem that
transformedCollection() would return a collection which was the result of
applying the transformer to the input collection.  Not so!  I've learned
that the hard way a few times (bad memory).

-----Original Message-----
From: Rob Oxspring [mailto:[hidden email]]
Sent: Saturday, May 07, 2005 11:42 AM
To: Jakarta Commons Developers List
Subject: [collections] TransformedCollection


Hi,

I've just been burnt (and not for the first time) by the fact that the
TransformedCollection doesn't transform the objects that are already in
the wrapped collection.  For example:

Collection dates = ... // a collection with Date objects in it Transformer
toString = ... // a transformer that calls toString Collection strings =
     CollectionUtils.transformedCollection(dates,toString);

My intuition tells me that strings should be full of String objects and
have no Date objects in it but the opposite is true.  Is this the
intended behaviour or is this a bug?  I'm happy to patch the
TransformedCollection class but wanted to run it passed people first...

Thanks,

Rob

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [collections] TransformedCollection

Rob Oxspring
Thanks James,

The latter scenario is exactly the one I keep finding myself in!  I
guess it's the naming that throws me - the collect() methods provide
what I'd expect of transformedCollection() under a name that seems
pretty abstract, whereas the implementation of transformedCollection()
seem more suited to a method called transformingCollection(), although
this could turn into a bike shed issue very quickly.

Seriously though, would it be reasonable to deprecate the collect()
methods in favour of something with 'transform' in the name? I'm not
sure what name to suggest but the word 'collect' might as well be
dropped as all CollectionUtils methods are implicitly about collecting :)

In the meantime, thanks for the pointer to the collect() family of
methods, I may well add some cross references in the javadocs between
these and the transformedCollection() method to try and avoid future
confusion.

Rob


James Carman wrote:

> Are you wanting to add dates to the resulting collection and have them
> transformed into Strings?  Or, are you just looking for a collection of
> Strings that you don't need to modify which represent the dates?  If it's
> the latter, you should use the CollectionUtils.collect() method rather than
> the transformedCollection() method.  I too have been burned by that very
> thing.  Just by looking at the name, it would seem that
> transformedCollection() would return a collection which was the result of
> applying the transformer to the input collection.  Not so!  I've learned
> that the hard way a few times (bad memory).
>
> -----Original Message-----
> From: Rob Oxspring [mailto:[hidden email]]
> Sent: Saturday, May 07, 2005 11:42 AM
> To: Jakarta Commons Developers List
> Subject: [collections] TransformedCollection
>
>
> Hi,
>
> I've just been burnt (and not for the first time) by the fact that the
> TransformedCollection doesn't transform the objects that are already in
> the wrapped collection.  For example:
>
> Collection dates = ... // a collection with Date objects in it Transformer
> toString = ... // a transformer that calls toString Collection strings =
>      CollectionUtils.transformedCollection(dates,toString);
>
> My intuition tells me that strings should be full of String objects and
> have no Date objects in it but the opposite is true.  Is this the
> intended behaviour or is this a bug?  I'm happy to patch the
> TransformedCollection class but wanted to run it passed people first...
>
> Thanks,
>
> Rob
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [collections] TransformedCollection

Stephen Colebourne
Really the problem is that the method transformedCollection() got added
to CollectionUtils at all.

Way back at the start of the debates, we had a choice between:
- CollectionUtils.transformedCollection()
- TransformedCollections.collection()
- Transformed.collection()
In the end we settled on the first, however I have on various occasions
felt the latter would have been better.

This then got more confused by the break out into subpackages in v3.0.
At that point, the 'best' way to create the class became:
- TransformedCollection.decorate()
however, we left the method on CollectionUtils for compatability.

Thus, there is an argument that says all the decoration methods on
CollectionUtils (and similar classes) should be deprecated. However,
personally I feel their use is now too widespread for this step.

Feel free to add more javadoc however, perhaps indicating the preferred
way to create these classes.

Stephen


Rob Oxspring wrote:

> Thanks James,
>
> The latter scenario is exactly the one I keep finding myself in!  I
> guess it's the naming that throws me - the collect() methods provide
> what I'd expect of transformedCollection() under a name that seems
> pretty abstract, whereas the implementation of transformedCollection()
> seem more suited to a method called transformingCollection(), although
> this could turn into a bike shed issue very quickly.
>
> Seriously though, would it be reasonable to deprecate the collect()
> methods in favour of something with 'transform' in the name? I'm not
> sure what name to suggest but the word 'collect' might as well be
> dropped as all CollectionUtils methods are implicitly about collecting :)
>
> In the meantime, thanks for the pointer to the collect() family of
> methods, I may well add some cross references in the javadocs between
> these and the transformedCollection() method to try and avoid future
> confusion.
>
> Rob
>
>
> James Carman wrote:
>
>> Are you wanting to add dates to the resulting collection and have them
>> transformed into Strings?  Or, are you just looking for a collection of
>> Strings that you don't need to modify which represent the dates?  If it's
>> the latter, you should use the CollectionUtils.collect() method rather
>> than
>> the transformedCollection() method.  I too have been burned by that very
>> thing.  Just by looking at the name, it would seem that
>> transformedCollection() would return a collection which was the result of
>> applying the transformer to the input collection.  Not so!  I've learned
>> that the hard way a few times (bad memory).
>>
>> -----Original Message-----
>> From: Rob Oxspring [mailto:[hidden email]] Sent: Saturday, May
>> 07, 2005 11:42 AM
>> To: Jakarta Commons Developers List
>> Subject: [collections] TransformedCollection
>>
>>
>> Hi,
>>
>> I've just been burnt (and not for the first time) by the fact that the
>> TransformedCollection doesn't transform the objects that are already
>> in the wrapped collection.  For example:
>>
>> Collection dates = ... // a collection with Date objects in it
>> Transformer
>> toString = ... // a transformer that calls toString Collection strings =
>>      CollectionUtils.transformedCollection(dates,toString);
>>
>> My intuition tells me that strings should be full of String objects
>> and have no Date objects in it but the opposite is true.  Is this the
>> intended behaviour or is this a bug?  I'm happy to patch the
>> TransformedCollection class but wanted to run it passed people first...
>>
>> Thanks,
>>
>> Rob
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [collections] TransformedCollection

Rob Oxspring
Does collections have a defined deprecation policy? I.e. have we made a
promise as to when they are to be deleted, were we to deprecate the
decoration methods in *Utils classes?  The way I see it, the wide spread
argument provides a good reason not to delete the methods in a hurry but
shouldn't stop us from using deprecation to recommend the 'correct' way
forward.

Rob

Stephen Colebourne wrote:

> Really the problem is that the method transformedCollection() got added
> to CollectionUtils at all.
>
> Way back at the start of the debates, we had a choice between:
> - CollectionUtils.transformedCollection()
> - TransformedCollections.collection()
> - Transformed.collection()
> In the end we settled on the first, however I have on various occasions
> felt the latter would have been better.
>
> This then got more confused by the break out into subpackages in v3.0.
> At that point, the 'best' way to create the class became:
> - TransformedCollection.decorate()
> however, we left the method on CollectionUtils for compatability.
>
> Thus, there is an argument that says all the decoration methods on
> CollectionUtils (and similar classes) should be deprecated. However,
> personally I feel their use is now too widespread for this step.
>
> Feel free to add more javadoc however, perhaps indicating the preferred
> way to create these classes.
>
> Stephen
>
>
> Rob Oxspring wrote:
>
>> Thanks James,
>>
>> The latter scenario is exactly the one I keep finding myself in!  I
>> guess it's the naming that throws me - the collect() methods provide
>> what I'd expect of transformedCollection() under a name that seems
>> pretty abstract, whereas the implementation of transformedCollection()
>> seem more suited to a method called transformingCollection(), although
>> this could turn into a bike shed issue very quickly.
>>
>> Seriously though, would it be reasonable to deprecate the collect()
>> methods in favour of something with 'transform' in the name? I'm not
>> sure what name to suggest but the word 'collect' might as well be
>> dropped as all CollectionUtils methods are implicitly about collecting :)
>>
>> In the meantime, thanks for the pointer to the collect() family of
>> methods, I may well add some cross references in the javadocs between
>> these and the transformedCollection() method to try and avoid future
>> confusion.
>>
>> Rob
>>
>>
>> James Carman wrote:
>>
>>> Are you wanting to add dates to the resulting collection and have them
>>> transformed into Strings?  Or, are you just looking for a collection of
>>> Strings that you don't need to modify which represent the dates?  If
>>> it's
>>> the latter, you should use the CollectionUtils.collect() method
>>> rather than
>>> the transformedCollection() method.  I too have been burned by that very
>>> thing.  Just by looking at the name, it would seem that
>>> transformedCollection() would return a collection which was the
>>> result of
>>> applying the transformer to the input collection.  Not so!  I've learned
>>> that the hard way a few times (bad memory).
>>>
>>> -----Original Message-----
>>> From: Rob Oxspring [mailto:[hidden email]] Sent: Saturday,
>>> May 07, 2005 11:42 AM
>>> To: Jakarta Commons Developers List
>>> Subject: [collections] TransformedCollection
>>>
>>>
>>> Hi,
>>>
>>> I've just been burnt (and not for the first time) by the fact that
>>> the TransformedCollection doesn't transform the objects that are
>>> already in the wrapped collection.  For example:
>>>
>>> Collection dates = ... // a collection with Date objects in it
>>> Transformer
>>> toString = ... // a transformer that calls toString Collection strings =
>>>      CollectionUtils.transformedCollection(dates,toString);
>>>
>>> My intuition tells me that strings should be full of String objects
>>> and have no Date objects in it but the opposite is true.  Is this the
>>> intended behaviour or is this a bug?  I'm happy to patch the
>>> TransformedCollection class but wanted to run it passed people first...
>>>
>>> Thanks,
>>>
>>> Rob
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [hidden email]
>>> For additional commands, e-mail: [hidden email]
>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [hidden email]
>>> For additional commands, e-mail: [hidden email]
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[PATCH] [EMAIL] Checkstyle errors cleaned up

Eric Spiegelberg
In reply to this post by James Carman
I read on the user list that Commons Email is approaching a v1.0 release
and decided to take a look at the Maven reports to see how close it was
and what needed to be done. I'm always somewhat disappointed when the
reports reflect that things aren't as tight as I've come to expect from
Jakarta software. Instead of groaning, I decided to do the clean up
myself. I am submitting a patch that fixes 184 of the192 Checkstyle
errors in Commons Email.

The patch includes trivial refactoring to use the Map interface instead
of Hashtable. Other things such as cleaning up or adding missing
javadoc, cleaning indentation, curly braces, parenthesis, and removing
tabs and trailing spaces at the end of lines. The 8 remaining errors
indicate that the file does not end with a newline, which they do. I
assume this is an error with the Maven-Checkstyle plugin.

It's not glamorous and it's not going to set anyone's world on fire, but
I felt it would be nice to have a clean Checkstyle report for a v1.0
release. This is my first submission to Jakarta and I welcome any
feedback in order to make this patch accepted smoothly.

Thanks,
Eric Spiegelberg


Index: src/test/org/apache/commons/mail/EmailTest.java
===================================================================
--- src/test/org/apache/commons/mail/EmailTest.java (revision 169394)
+++ src/test/org/apache/commons/mail/EmailTest.java (working copy)
@@ -21,6 +21,8 @@
 import java.util.Date;
 import java.util.Enumeration;
 import java.util.Hashtable;
+import java.util.Iterator;
+import java.util.Map;
 import java.util.Properties;
 
 import javax.mail.Authenticator;
@@ -1289,25 +1291,25 @@
         // ====================================================================
         // Test Success
         // ====================================================================
-        Hashtable ht = new Hashtable();
-        ht.put("X-Priority", "1");
-        ht.put("Disposition-Notification-To", "[hidden email]");
-        ht.put("X-Mailer", "Sendmail");
+        Map m = new Hashtable();
 
-        Enumeration enumKey = ht.keys();
+        m.put("X-Priority", "1");
+        m.put("Disposition-Notification-To", "[hidden email]");
+        m.put("X-Mailer", "Sendmail");
 
-        while (enumKey.hasMoreElements())
+        Iterator iterKey = m.keySet().iterator();
+        while (iterKey.hasNext())
         {
-            String strName = (String) enumKey.nextElement();
-            String strValue = (String) ht.get(strName);
+            String strName = (String) iterKey.next();
+            String strValue = (String) m.get(strName);
 
             this.email.addHeader(strName, strValue);
         }
 
-        assertEquals(ht.size(), this.email.getHeaders().size());
-        assertEquals(ht, this.email.getHeaders());
+        assertEquals(m.size(), this.email.getHeaders().size());
+        assertEquals(m, this.email.getHeaders());
     }
-
+    
     /** */
     public void testAddHeaderEx()
     {
Index: src/test/org/apache/commons/mail/mocks/MockEmailConcrete.java
===================================================================
--- src/test/org/apache/commons/mail/mocks/MockEmailConcrete.java (revision 169394)
+++ src/test/org/apache/commons/mail/mocks/MockEmailConcrete.java (working copy)
@@ -15,8 +15,8 @@
  */
 package org.apache.commons.mail.mocks;
 
-import java.util.Hashtable;
 import java.util.List;
+import java.util.Map;
 
 import javax.mail.Authenticator;
 import javax.mail.Session;
@@ -128,7 +128,7 @@
     /**
      * @return headers
      */
-    public Hashtable getHeaders()
+    public Map getHeaders()
     {
         return this.headers;
     }
Index: src/java/org/apache/commons/mail/EmailException.java
===================================================================
--- src/java/org/apache/commons/mail/EmailException.java (revision 169394)
+++ src/java/org/apache/commons/mail/EmailException.java (working copy)
@@ -1,7 +1,7 @@
 /*
  * Copyright 2001-2004 The Apache Software Foundation
  *
- * Licensed under the Apache License, Version 2.0 ( the "License" );
+ * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
  *
@@ -21,25 +21,25 @@
  * EmailException
  * @author jakarta-commons
  */
-public class EmailException extends NestableException
+public class EmailException extends NestableException
 {
     /** */
     public EmailException()
     {
         super();
     }
-    
+
     /**
-     *
+     *
      * @param msg msg
      */
     public EmailException(String msg)
     {
         super(msg);
     }
-    
+
     /**
-     *
+     *
      * @param msg msg
      * @param cause cause
      */
@@ -47,14 +47,14 @@
     {
         super(msg, cause);
     }
-    
+
     /**
-     *
+     *
      * @param cause cause
      */
     public EmailException(Throwable cause)
     {
         super(cause);
     }
-    
+
 }
Index: src/java/org/apache/commons/mail/Email.java
===================================================================
--- src/java/org/apache/commons/mail/Email.java (revision 169394)
+++ src/java/org/apache/commons/mail/Email.java (working copy)
@@ -1,7 +1,7 @@
 /*
  * Copyright 2001-2005 The Apache Software Foundation
  *
- * Licensed under the Apache License, Version 2.0 ( the "License" );
+ * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
  *
@@ -18,9 +18,10 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Date;
-import java.util.Enumeration;
 import java.util.Hashtable;
+import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 import java.util.Properties;
 
 import javax.mail.Authenticator;
@@ -125,14 +126,11 @@
     protected String contentType;
 
     /** Set session debugging on or off */
-    protected boolean debug = false;
+    protected boolean debug;
 
     /** Sent date */
     protected Date sentDate;
 
-    /** The Session to mail with */
-    private Session session;
-
     /**
      * Instance of an <code>Authenticator</code> object that will be used
      * when authentication is requested from the mail server.
@@ -146,7 +144,7 @@
     protected String hostName;
 
     /**
-     * The port number of the mail server to connect to.  
+     * The port number of the mail server to connect to.
      * Defaults to the standard port ( 25 ).
      */
     protected String smtpPort = "25";
@@ -163,13 +161,13 @@
     /** List of "replyTo" email adresses */
     protected List replyList = new ArrayList();
 
-    /**
-     * Address to which undeliverable mail should be sent.
+    /**
+     * Address to which undeliverable mail should be sent.
      * Because this is handled by JavaMail as a String property
      * in the mail session, this property is of type <code>String</code>
      * rather than <code>InternetAddress</code>.
      */
-    protected String bounceAddress = null;
+    protected String bounceAddress;
 
     /**
      * Used to specify the mail headers.  Example:
@@ -178,21 +176,24 @@
      * or  2( high ) 3( normal ) 4( low ) and 5( lowest )
      * Disposition-Notification-To: [hidden email]
      */
-    protected Hashtable headers = new Hashtable();
+    protected Map headers = new Hashtable();
 
     /**
      * Used to determine whether to use pop3 before smtp, and if so the settings.
      */
 
     /** */
-    protected boolean popBeforeSmtp = false;
+    protected boolean popBeforeSmtp;
     /** */
-    protected String popHost = null;
+    protected String popHost;
     /** */
-    protected String popUsername = null;
+    protected String popUsername;
     /** */
-    protected String popPassword = null;
+    protected String popPassword;
 
+    /** The Session to mail with */
+    private Session session;
+
     /**
      * Setting to true will enable the display of debug information.
      *
@@ -337,7 +338,7 @@
      * Initialise a mailsession object
      *
      * @return A Session.
-     * @throws EmailException thrown when host name was not set
+     * @throws EmailException thrown when host name was not set.
      */
     protected Session getMailSession() throws EmailException
     {
@@ -371,34 +372,35 @@
                 properties.setProperty(MAIL_SMTP_FROM, this.bounceAddress);
             }
 
-            // changed this (back) to getInstance due to security exceptions
+            // changed this (back) to getInstance due to security exceptions
             // caused when testing using maven
             this.session =
                 Session.getInstance(properties, this.authenticator);
         }
         return this.session;
     }
-    
+
     /**
-    * Creates a InternetAddress
-    * @param email An Email
-    * @param name A Name.  
-    * @return An internet address
-    * @throws EmailException thrown when the address supplied or name were invalid
-    */
-    private InternetAddress createInternetAddress(String email, String name)
+     * Creates a InternetAddress
+     *
+     * @param email An email address.
+     * @param name A name.
+     * @return An internet address.
+     * @throws EmailException Thrown when the address supplied or name were invalid.
+     */
+    private InternetAddress createInternetAddress(String email, String name)
         throws EmailException
     {
         InternetAddress address = null;
-        
-        try
+
+        try
         {
             // check name input
             if (!StringUtils.isNotEmpty(name))
             {
                 name = email;
             }
-            
+
             if (StringUtils.isNotEmpty(this.charset))
             {
                 address = new InternetAddress(email, name, this.charset);
@@ -407,7 +409,7 @@
             {
                 address = new InternetAddress(email, name);
             }
-            
+
             address.validate();
         }
         catch (Exception e)
@@ -416,17 +418,17 @@
         }
         return address;
     }
-    
-    
+
+
     /**
      * Set the FROM field of the email.
      *
      * @param email A String.
      * @return An Email.
-     * @throws EmailException Indicates an invalid email address
+     * @throws EmailException Indicates an invalid email address.
      */
-    public Email setFrom(String email)
-        throws EmailException
+    public Email setFrom(String email)
+        throws EmailException
     {
         return setFrom(email, null);
     }
@@ -436,10 +438,10 @@
      *
      * @param email A String.
      * @param name A String.
+     * @throws EmailException Indicates an invalid email address.
      * @return An Email.
-     * @throws EmailException Indicates an invalid email address
      */
-    public Email setFrom(String email, String name)
+    public Email setFrom(String email, String name)
         throws EmailException
     {
         this.fromAddress = createInternetAddress(email, name);
@@ -451,10 +453,10 @@
      * Add a recipient TO to the email.
      *
      * @param email A String.
+     * @throws EmailException Indicates an invalid email address.
      * @return An Email.
-     * @throws EmailException Indicates an invalid email address
      */
-    public Email addTo(String email)
+    public Email addTo(String email)
         throws EmailException
     {
         return addTo(email, null);
@@ -465,23 +467,22 @@
      *
      * @param email A String.
      * @param name A String.
+     * @throws EmailException Indicates an invalid email address.
      * @return An Email.
-     * @throws EmailException Indicates an invalid email address
      */
-    public Email addTo(String email, String name)
+    public Email addTo(String email, String name)
         throws EmailException
     {
         this.toList.add(createInternetAddress(email, name));
-
         return this;
     }
 
     /**
      * Set a list of "TO" addresses
      *
-     * @param   aCollection collection of InternetAddress objects
+     * @param  aCollection collection of InternetAddress objects
+     * @throws EmailException Indicates an invalid email address.
      * @return An Email.
-     * @throws EmailException Indicates an invalid email address
      */
     public Email setTo(Collection aCollection) throws EmailException
     {
@@ -499,9 +500,9 @@
      *
      * @param email A String.
      * @return An Email.
-     * @throws EmailException Indicates an invalid email address
+     * @throws EmailException Indicates an invalid email address.
      */
-    public Email addCc(String email)
+    public Email addCc(String email)
         throws EmailException
     {
         return this.addCc(email, null);
@@ -512,22 +513,22 @@
      *
      * @param email A String.
      * @param name A String.
+     * @throws EmailException Indicates an invalid email address.
      * @return An Email.
-     * @throws EmailException Indicates an invalid email address
+
      */
-    public Email addCc(String email, String name)
+    public Email addCc(String email, String name)
         throws EmailException
     {
         this.ccList.add(createInternetAddress(email, name));
-
         return this;
     }
 
     /**
      * Set a list of "CC" addresses
      *
-     * @param   aCollection collection of InternetAddress objects
-     * @return An EmailException.
+     * @param aCollection collection of InternetAddress objects
+     * @return An Email.
      * @throws EmailException Indicates an invalid email address
      */
     public Email setCc(Collection aCollection) throws EmailException
@@ -548,7 +549,7 @@
      * @return An Email.
      * @throws EmailException Indicates an invalid email address
      */
-    public Email addBcc(String email)
+    public Email addBcc(String email)
         throws EmailException
     {
         return this.addBcc(email, null);
@@ -562,12 +563,10 @@
      * @return An Email.
      * @throws EmailException Indicates an invalid email address
      */
-    public Email addBcc(String email, String name)
+    public Email addBcc(String email, String name)
         throws EmailException
     {
-
         this.bccList.add(createInternetAddress(email, name));
-
         return this;
     }
 
@@ -596,7 +595,7 @@
      * @return An Email.
      * @throws EmailException Indicates an invalid email address
      */
-    public Email addReplyTo(String email)
+    public Email addReplyTo(String email)
         throws EmailException
     {
         return this.addReplyTo(email, null);
@@ -613,9 +612,7 @@
     public Email addReplyTo(String email, String name)
         throws EmailException
     {
-        
         this.replyList.add(createInternetAddress(email, name));
-
         return this;
     }
 
@@ -626,16 +623,16 @@
      * or  2( high ) 3( normal ) 4( low ) and 5( lowest )
      * Disposition-Notification-To: [hidden email]
      *
-     * @param ht A Hashtable.
+     * @param map A Map.
      */
-    public void setHeaders(Hashtable ht)
+    public void setHeaders(Map map)
     {
-        Enumeration enumKeyBad = ht.keys();
+        Iterator iterKeyBad = map.keySet().iterator();
 
-        while (enumKeyBad.hasMoreElements())
+        while (iterKeyBad.hasNext())
         {
-            String strName = (String) enumKeyBad.nextElement();
-            String strValue = (String) ht.get(strName);
+            String strName = (String) iterKeyBad.next();
+            String strValue = (String) map.get(strName);
 
             if (!StringUtils.isNotEmpty(strName))
             {
@@ -648,7 +645,7 @@
         }
 
         // all is ok, update headers
-        this.headers = ht;
+        this.headers = map;
     }
 
     /**
@@ -705,7 +702,7 @@
      *
      * @param msg A String.
      * @return An Email.
-     * @throws EmailException generic exception
+     * @throws EmailException generic exception.
      */
     public abstract Email setMsg(String msg) throws EmailException;
 
@@ -716,11 +713,11 @@
      */
     public void send() throws EmailException
     {
-        try
+        try
         {
             this.getMailSession();
             this.message = new MimeMessage(this.session);
-    
+
             if (StringUtils.isNotEmpty(this.subject))
             {
                 if (StringUtils.isNotEmpty(this.charset))
@@ -732,7 +729,7 @@
                     this.message.setSubject(this.subject);
                 }
             }
-    
+
             // ========================================================
             // Start of replacement code
             if (this.content != null)
@@ -749,7 +746,7 @@
             {
                 this.message.setContent("", Email.TEXT_PLAIN);
             }
-    
+
             if (this.fromAddress != null)
             {
                 this.message.setFrom(this.fromAddress);
@@ -758,69 +755,68 @@
             {
                 throw new EmailException("Sender address required");
             }
-    
+
             if (this.toList.size() + this.ccList.size() + this.bccList.size() == 0)
             {
                 throw new EmailException(
                             "At least one receiver address required");
             }
-    
+
             if (this.toList.size() > 0)
             {
                 this.message.setRecipients(
                     Message.RecipientType.TO,
                     this.toInternetAddressArray(this.toList));
             }
-    
+
             if (this.ccList.size() > 0)
             {
                 this.message.setRecipients(
                     Message.RecipientType.CC,
                     this.toInternetAddressArray(this.ccList));
             }
-    
+
             if (this.bccList.size() > 0)
             {
                 this.message.setRecipients(
                     Message.RecipientType.BCC,
                     this.toInternetAddressArray(this.bccList));
             }
-    
+
             if (this.replyList.size() > 0)
             {
                 this.message.setReplyTo(
                     this.toInternetAddressArray(this.replyList));
             }
-    
+
             if (this.headers.size() > 0)
             {
-                Enumeration enumHeaderKeys = this.headers.keys();
-    
-                while (enumHeaderKeys.hasMoreElements())
+                Iterator iterHeaderKeys = this.headers.keySet().iterator();
+                while (iterHeaderKeys.hasNext())
                 {
-                    String name = (String) enumHeaderKeys.nextElement();
+                    String name = (String) iterHeaderKeys.next();
                     String value = (String) headers.get(name);
                     this.message.addHeader(name, value);
                 }
             }
-    
+
             if (this.message.getSentDate() == null)
             {
                 this.message.setSentDate(getSentDate());
             }
-    
+
             if (this.popBeforeSmtp)
             {
                 Store store = session.getStore("pop3");
                 store.connect(this.popHost, this.popUsername, this.popPassword);
             }
-    
+
             Transport.send(this.message);
         }
         catch (MessagingException me)
         {
             throw new EmailException(me);
-        }    
+        }
     }
 
     /**
@@ -855,20 +851,20 @@
      * Utility to copy List of known InternetAddress objects into an
      * array.
      *
-     * @param aList A List of InternetAddress.
+     * @param list A List.
      * @return An InternetAddress[].
      */
-    protected InternetAddress[] toInternetAddressArray(List aList)
+    protected InternetAddress[] toInternetAddressArray(List list)
     {
         InternetAddress[] ia =
-            (InternetAddress[]) aList.toArray(new InternetAddress[0]);
+            (InternetAddress[]) list.toArray(new InternetAddress[0]);
 
         return ia;
     }
 
     /**
      * Set details regarding "pop3 before smtp" authentication
-     * @param newPopBeforeSmtp Wether or not to log into pop3
+     * @param newPopBeforeSmtp Wether or not to log into pop3
      *      server before sending mail
      * @param newPopHost The pop3 host to use.
      * @param newPopUsername The pop3 username.
Index: src/java/org/apache/commons/mail/MultiPartEmail.java
===================================================================
--- src/java/org/apache/commons/mail/MultiPartEmail.java (revision 169394)
+++ src/java/org/apache/commons/mail/MultiPartEmail.java (working copy)
@@ -49,22 +49,23 @@
 public class MultiPartEmail extends Email
 {
     /** Body portion of the email. */
-    private MimeMultipart container = null;
+    private MimeMultipart container;
 
     /** The message container. */
-    private MimeBodyPart primaryBodyPart = null;
+    private MimeBodyPart primaryBodyPart;
 
     /** The MIME subtype. */
-    private String subType = null;
+    private String subType;
 
     /** Indicates if the message has been initialized */
-    private boolean initialized = false;
+    private boolean initialized;
 
     /** Indicates if attachments have been added to the message */
-    private boolean boolHasAttachments = false;
-    
+    private boolean boolHasAttachments;
+
     /**
      * Set the MIME subtype of the email.
+     *
      * @param aSubType MIME subtype of the email
      */
     public void setSubType(String aSubType)
@@ -74,6 +75,7 @@
 
     /**
      * Get the MIME subtype of the email.
+     *
      * @return MIME subtype of the email
      */
     public String getSubType()
@@ -83,6 +85,7 @@
 
     /**
      * Add a new part to the email.
+     *
      * @param content The content.
      * @param contentType The content type.
      * @return An Email.
@@ -108,6 +111,7 @@
 
     /**
      * Add a new part to the email.
+     *
      * @param multipart The MimeMultipart.
      * @return An Email.
      * @throws EmailException see javax.mail.internet.MimeBodyPart
@@ -115,22 +119,34 @@
      */
     public Email addPart(MimeMultipart multipart) throws EmailException
     {
-        try{
+        try
+        {
             return addPart(multipart, getContainer().getCount());
         }
-        catch( MessagingException me ){
+        catch (MessagingException me)
+        {
             throw new EmailException(me);
         }
     }
 
+    /**
+     * Add a new part to the email.
+     *
+     * @param multipart The part to add.
+     * @param index The index to add at.
+     * @return The email.
+     * @throws EmailException An error occured while adding the part.
+     */
     public Email addPart(MimeMultipart multipart, int index) throws EmailException
     {
         MimeBodyPart bodyPart = new MimeBodyPart();
-        try {
+        try
+        {
             bodyPart.setContent(multipart);
             getContainer().addBodyPart(bodyPart, index);
         }
-        catch (MessagingException me){
+        catch (MessagingException me)
+        {
             throw new EmailException(me);
         }
 
@@ -139,9 +155,6 @@
 
     /**
      * Initialize the multipart email.
-     *
-     * @throws EmailException see javax.mail.internet.MimeBodyPart
-     *  for defintions
      */
     protected void init()
     {
@@ -151,7 +164,7 @@
         }
 
         container = new MimeMultipart();
-        super.setContent(container);      
+        super.setContent(container);
 
         initialized = true;
     }
@@ -171,7 +184,8 @@
         {
             throw new EmailException("Invalid message supplied");
         }
-        try {
+        try
+        {
             if (StringUtils.isNotEmpty(charset))
             {
                 getPrimaryBodyPart().setText(msg, charset);
@@ -181,9 +195,10 @@
                 getPrimaryBodyPart().setText(msg);
             }
         }
-        catch (MessagingException me){
+        catch (MessagingException me)
+        {
             throw new EmailException(me);
-        }  
+        }
         return this;
     }
 
@@ -195,15 +210,16 @@
      */
     public void send() throws EmailException
     {
-        try {
+        try
+        {
             if (primaryBodyPart != null)
             {
                 // before a multipart message can be sent, we must make sure that
                 // the content for the main body part was actually set.  If not,
                 // an IOException will be thrown during super.send().
-    
+
                 MimeBodyPart body = this.getPrimaryBodyPart();
-                Object content = null;
+                //Object content = null;
                 try
                 {
                     content = body.getContent();
@@ -215,15 +231,16 @@
                     content = null;
                 }
             }
-    
+
             if (subType != null)
             {
                 getContainer().setSubType(subType);
             }
-    
+
             super.send();
         }
-        catch (MessagingException me){
+        catch (MessagingException me)
+        {
             throw new EmailException(me);
         }
     }
@@ -247,7 +264,7 @@
         }
 
         URL url = attachment.getURL();
-        
+
         if (url == null)
         {
             String fileName = null;
@@ -391,7 +408,8 @@
             name = ds.getName();
         }
         MimeBodyPart mbp = new MimeBodyPart();
-        try {
+        try
+        {
             getContainer().addBodyPart(mbp);
 
             mbp.setDisposition(disposition);
@@ -399,11 +417,12 @@
             mbp.setDescription(description);
             mbp.setDataHandler(new DataHandler(ds));
         }
-        catch (MessagingException me){
+        catch (MessagingException me)
+        {
             throw new EmailException(me);
-        }            
+        }
         this.boolHasAttachments = true;
-        
+
         return this;
     }
 
@@ -411,8 +430,7 @@
      * Gets first body part of the message.
      *
      * @return The primary body part.
-     * @throws EmailException see javax.mail.internet.MimeBodyPart
-     *  for defintions
+     * @throws MessagingException An error occured while getting the primary body part.
      */
     protected MimeBodyPart getPrimaryBodyPart() throws MessagingException
     {
@@ -420,7 +438,7 @@
         {
             init();
         }
-        
+
         // Add the first body part to the message.  The fist body part must be
         if (this.primaryBodyPart == null)
         {
@@ -435,8 +453,6 @@
      * Gets the message container.
      *
      * @return The message container.
-     * @throws EmailException see javax.mail.internet.MimeBodyPart
-     *  for defintions
      */
     protected MimeMultipart getContainer()
     {
@@ -446,8 +462,8 @@
         }
         return container;
     }
-    
 
+
     /**
      * @return boolHasAttachments
      */
@@ -463,5 +479,4 @@
     {
         boolHasAttachments = b;
     }
-
-}
+}
\ No newline at end of file
Index: src/java/org/apache/commons/mail/HtmlEmail.java
===================================================================
--- src/java/org/apache/commons/mail/HtmlEmail.java (revision 169394)
+++ src/java/org/apache/commons/mail/HtmlEmail.java (working copy)
@@ -57,6 +57,9 @@
  */
 public class HtmlEmail extends MultiPartEmail
 {
+    /** Defintion of the length of generated CID's */
+    public static final int CID_LENGTH = 10;
+
     /**
      * Text part of the message.  This will be used as alternative text if
      * the email client does not support HTML messages.
@@ -69,9 +72,6 @@
     /** Embeded images */
     protected List inlineImages = new ArrayList();
 
-    /** Defintion of the length of generated CID's */
-    public static final int CID_LENGTH = 10;
-
     /**
      * Set the text content.
      *
@@ -105,7 +105,7 @@
         {
             throw new EmailException("Invalid message supplied");
         }
-        
+
         this.html = aHtml;
         return this;
     }
@@ -190,13 +190,12 @@
             String cid = RandomStringUtils.randomAlphabetic(HtmlEmail.CID_LENGTH).toLowerCase();
             mbp.addHeader("Content-ID", "<" + cid + ">");
             this.inlineImages.add(mbp);
-            return cid;            
-        }
+            return cid;
+        }
         catch (MessagingException me)
         {
             throw new EmailException(me);
         }
-        
     }
 
     /**
@@ -208,7 +207,7 @@
     {
         try
         {
-            // if the email has attachments then the base type is mixed,
+            // if the email has attachments then the base type is mixed,
             // otherwise it should be related
             if (this.isBoolHasAttachments())
             {
@@ -235,7 +234,7 @@
     {
         MimeMultipart container = this.getContainer();
         MimeMultipart subContainer = null;
- MimeMultipart subContainerHTML = new MimeMultipart("related");
+        MimeMultipart subContainerHTML = new MimeMultipart("related");
         BodyPart msgHtml = null;
         BodyPart msgText = null;
 
@@ -261,16 +260,16 @@
 
         if (StringUtils.isNotEmpty(this.html))
         {
- if (this.inlineImages.size() > 0)
- {
- msgHtml = new MimeBodyPart();
- subContainerHTML.addBodyPart(msgHtml);
- }
- else
- {
- msgHtml = new MimeBodyPart();
- subContainer.addBodyPart(msgHtml);
- }
+            if (this.inlineImages.size() > 0)
+            {
+                msgHtml = new MimeBodyPart();
+                subContainerHTML.addBodyPart(msgHtml);
+            }
+            else
+            {
+                msgHtml = new MimeBodyPart();
+                subContainer.addBodyPart(msgHtml);
+            }
 
             if (StringUtils.isNotEmpty(this.charset))
             {
@@ -284,20 +283,20 @@
             }
 
             Iterator iter = this.inlineImages.iterator();
- while (iter.hasNext())
- {
- subContainerHTML.addBodyPart((BodyPart) iter.next());
- }
+            while (iter.hasNext())
+            {
+                subContainerHTML.addBodyPart((BodyPart) iter.next());
+            }
         }
 
         // add sub containers to message
         this.addPart(subContainer, 0);
 
- if (this.inlineImages.size() > 0)
- {
- // add sub container to message
- this.addPart(subContainerHTML, 1);
- }
+        if (this.inlineImages.size() > 0)
+        {
+            // add sub container to message
+            this.addPart(subContainerHTML, 1);
+        }
     }
 
     /**
Index: src/java/org/apache/commons/mail/ByteArrayDataSource.java
===================================================================
--- src/java/org/apache/commons/mail/ByteArrayDataSource.java (revision 169394)
+++ src/java/org/apache/commons/mail/ByteArrayDataSource.java (working copy)
@@ -40,15 +40,15 @@
  */
 public class ByteArrayDataSource implements DataSource
 {
+    /** define the buffer size */
+    public static final int BUFFER_SIZE = 512;
+
     /** Stream containg the Data */
-    private ByteArrayOutputStream baos = null;
+    private ByteArrayOutputStream baos;
 
     /** Content-type. */
     private String type = "application/octet-stream";
 
-    /** define the buffer size */
-    public static final int BUFFER_SIZE = 512;
-
     /**
      * Create a datasource from a byte array.
      *
@@ -91,6 +91,41 @@
     }
 
     /**
+     * Create a datasource from a String.
+     *
+     * @param data A String.
+     * @param aType A String.
+     * @throws IOException IOException
+     */
+    public ByteArrayDataSource(String data, String aType) throws IOException
+    {
+        this.type = aType;
+
+        try
+        {
+            baos = new ByteArrayOutputStream();
+
+            // Assumption that the string contains only ASCII
+            // characters!  Else just pass in a charset into this
+            // constructor and use it in getBytes().
+            baos.write(data.getBytes("iso-8859-1"));
+            baos.flush();
+            baos.close();
+        }
+        catch (UnsupportedEncodingException uex)
+        {
+            throw new IOException("The Character Encoding is not supported.");
+        }
+        finally
+        {
+            if (baos != null)
+            {
+                baos.close();
+            }
+        }
+    }
+
+    /**
       * Create a datasource from an input stream.
       *
       * @param aIs An InputStream.
@@ -144,41 +179,8 @@
         }
     }
 
-    /**
-     * Create a datasource from a String.
-     *
-     * @param data A String.
-     * @param aType A String.
-     * @throws IOException IOException
-     */
-    public ByteArrayDataSource(String data, String aType) throws IOException
-    {
-        this.type = aType;
 
-        try
-        {
-            baos = new ByteArrayOutputStream();
 
-            // Assumption that the string contains only ASCII
-            // characters!  Else just pass in a charset into this
-            // constructor and use it in getBytes().
-            baos.write(data.getBytes("iso-8859-1"));
-            baos.flush();
-            baos.close();
-        }
-        catch (UnsupportedEncodingException uex)
-        {
-            throw new IOException("The Character Encoding is not supported.");
-        }
-        finally
-        {
-            if (baos != null)
-            {
-                baos.close();
-            }
-        }
-    }
-
     /**
      * Get the content type.
      *
@@ -186,7 +188,7 @@
      */
     public String getContentType()
     {
-        return (type == null ? "application/octet-stream" : type);
+        return type == null ? "application/octet-stream" : type;
     }
 
     /**


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] [EMAIL] Checkstyle errors cleaned up

Corey Scott
Eric,

Firstly, welcome... Secondly... thanks.

There are two things that would like to suggest with your patch.

First:
Please try to break this patch down into discrete sections.  i.e. one
patch for formatting and one for each of the other changes.

Second:
Please submit your patch(es) to the bugzilla as files.  This allows
use to keep everything in one place.

Again, welcome and thanks.
Corey

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] [EMAIL] Checkstyle errors cleaned up

Matt Benson
In reply to this post by Eric Spiegelberg
I just joined the list myself... (for sandbox
commons-pgp) but I noticed that in the non-checkstyle
changes from this patch, the signature of a public
method is modified in at least one place. This will
break already-compiled code running against the
library.  Over in Ant-land we consider that bad; I can
only assume the same would be true in Jakarta commons.

$0.02,
Matt


--- Corey Scott <[hidden email]> wrote:

> Eric,
>
> Firstly, welcome... Secondly... thanks.
>
> There are two things that would like to suggest with
> your patch.
>
> First:
> Please try to break this patch down into discrete
> sections.  i.e. one
> patch for formatting and one for each of the other
> changes.
>
> Second:
> Please submit your patch(es) to the bugzilla as
> files.  This allows
> use to keep everything in one place.
>
> Again, welcome and thanks.
> Corey
>
>
---------------------------------------------------------------------
> To unsubscribe, e-mail:
> [hidden email]
> For additional commands, e-mail:
> [hidden email]
>
>



               
Discover Yahoo!
Stay in touch with email, IM, photo sharing and more. Check it out!
http://discover.yahoo.com/stayintouch.html

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] [EMAIL] Checkstyle errors cleaned up

Dion Gillard-2
Ok, it sounds like we need to get another patch, or manually separate
out the checkstyle from the changes.

Eric, would you mind creating another two patches and attaching them
in a post to the list??

Thanks,

On 5/11/05, Matt Benson <[hidden email]> wrote:

> I just joined the list myself... (for sandbox
> commons-pgp) but I noticed that in the non-checkstyle
> changes from this patch, the signature of a public
> method is modified in at least one place. This will
> break already-compiled code running against the
> library.  Over in Ant-land we consider that bad; I can
> only assume the same would be true in Jakarta commons.
>
> $0.02,
> Matt
>
>
> --- Corey Scott <[hidden email]> wrote:
> > Eric,
> >
> > Firstly, welcome... Secondly... thanks.
> >
> > There are two things that would like to suggest with
> > your patch.
> >
> > First:
> > Please try to break this patch down into discrete
> > sections.  i.e. one
> > patch for formatting and one for each of the other
> > changes.
> >
> > Second:
> > Please submit your patch(es) to the bugzilla as
> > files.  This allows
> > use to keep everything in one place.
> >
> > Again, welcome and thanks.
> > Corey
> >
> >
> ---------------------------------------------------------------------
> > To unsubscribe, e-mail:
> > [hidden email]
> > For additional commands, e-mail:
> > [hidden email]
> >
> >
>
> Discover Yahoo!
> Stay in touch with email, IM, photo sharing and more. Check it out!
> http://discover.yahoo.com/stayintouch.html
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


--
http://www.multitask.com.au/people/dion/

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] [EMAIL] Checkstyle errors cleaned up

Eric Spiegelberg
In reply to this post by Matt Benson
I agree with you that breaking existing API's is a bad thing. However,
the project is currently at 1.0-dev and the main page itself states:

    * The code is unreleased
    * Methods and classes can and will appear and disappear without warning

While changing the API at this point is less than ideal, most people
would accept that there exists a potential of change between 1.0-dev and
1.0. When it comes down to it, all we're talking about is changing a
public method's return object from a Hashtable to a Map. This is likely
the last opportunity to make this (trivial) change before the v1.0
release and it would then have to be postponed until 1.1 (or whatever).
I think changing it now is in everyone's best interest.

Although I disagree with you in this specific circumstance, you've got a
great point in general. This weekend I plan to follow Corey's feedback
and break the patch down into discrete parts, one for Checkstyle clean
up and one for this code change. That way, the commiters have the
freedom to decide if they want to break the API now or down the road.

Thanks for the food for thought,
Eric

Eric Spiegelberg wrote:

> Matt Benson wrote:
>
>>I just joined the list myself... (for sandbox
>>commons-pgp) but I noticed that in the non-checkstyle
>>changes from this patch, the signature of a public
>>method is modified in at least one place. This will
>>break already-compiled code running against the
>>library.  Over in Ant-land we consider that bad; I can
>>only assume the same would be true in Jakarta commons.
>>
>>$0.02,
>>Matt
>>
>>
>>--- Corey Scott <[hidden email]> wrote:
>>  
>>
>>>Eric,
>>>
>>>Firstly, welcome... Secondly... thanks.
>>>
>>>There are two things that would like to suggest with
>>>your patch.
>>>
>>>First:
>>>Please try to break this patch down into discrete
>>>sections.  i.e. one
>>>patch for formatting and one for each of the other
>>>changes.
>>>
>>>Second:
>>>Please submit your patch(es) to the bugzilla as
>>>files.  This allows
>>>use to keep everything in one place.
>>>
>>>Again, welcome and thanks.
>>>Corey
>>>
>>>
>>>    
>>>
>>---------------------------------------------------------------------
>>  
>>
>>>To unsubscribe, e-mail:
>>>[hidden email]
>>>For additional commands, e-mail:
>>>[hidden email]
>>>
>>>
>>>    
>>>
>>
>>
>>
>>
>>Discover Yahoo!
>>Stay in touch with email, IM, photo sharing and more. Check it out!
>>http://discover.yahoo.com/stayintouch.html
>>
>>---------------------------------------------------------------------
>>To unsubscribe, e-mail: [hidden email]
>>For additional commands, e-mail: [hidden email]
>>
>>
>>
>>
>>  
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] [EMAIL] Checkstyle errors cleaned up

Matt Benson
In reply to this post by Eric Spiegelberg
Sorry about having taken several days to respond
here... I was unaware of the release status of the
code in question and my response was mainly reflex.  I
agree that making a public API as generic as possible
before an official release is the right thing to do.
Jakarta commons is, of course, made up of many
constituent components in various states and I will
stay cognizant of that in the future.

-Matt

--- Eric Spiegelberg <[hidden email]> wrote:
> I agree with you that breaking existing API's is a
> bad thing. However,
> the project is currently at 1.0-dev and the main
> page itself states:
>
>     * The code is unreleased
>     * Methods and classes can and will appear and
> disappear without warning
> [SNIP]


               
Yahoo! Mail
Stay connected, organized, and protected. Take the tour:
http://tour.mail.yahoo.com/mailtour.html


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]