geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jason Dillon <ja...@planet57.com>
Subject Re: svn commit: r479586 - /geronimo/server/trunk/modules/geronimo-common/src/main/java/org/apache/geronimo/common/propertyeditor/MapEditor.java
Date Tue, 28 Nov 2006 06:05:03 GMT
I don't think it is appropriate to simply copy/paste the jira content  
into the svn change comment.  I expect the jira issue to contain a  
lot more details and some comments about the change, where I expect  
the svn change comment to briefly explain the change.

--jason


On Nov 27, 2006, at 9:57 PM, Vamsavardhana Reddy wrote:

> Hi Jason,
>
> Sometime ago I had difficulty in figuring out what revision(s)  
> fixed a particular JIRA and vice versa.  So, I have made it a  
> practice to include the JIRA number in the change log and svn  
> revision numbers in the JIRA comments so that someone investigating  
> the JIRA or the svn revision will be able to figureout what is  
> happening either from the JIRA or the svn change log without much  
> difficulty.  I thought this is complete & sufficient and is  
> definitely an improvement on what was followed earlier.  For those  
> revisions that are not related to a JIRA, I have included a  
> description of the change in the change log itself.  Since no one  
> had any complaints on this, I have followed it till now.  Adding  
> full details (or a summary) of the change to the change log would  
> not be much of a overhead and it amounts to one more Copy+Paste.  I  
> agree that adding a summary of changes along with a link to where  
> full details can be found (like the JIRA number) is definitely a  
> further improvement and will be followed from now on.  After all,  
> all our efforts are to improve things and make life easy (for a lot  
> of others).
>
> --vamsi
>
> On 11/28/06, Jason Dillon <jason@planet57.com> wrote:
> Please... please... please... put more context into the commit log.
>
> "MapEditor does not work" tells me nothing about what was actually
> changed.  When auditing changes it helps to have some context to the
> change in the commit log so that its easy to comprehend what the
> change was, with out having to dig around, or bounce into JIRA, etc.
>
> I've mentioned this a few times before... while its is good to
> include the JIRA issue ID, only including that ID, or in this case
> the ID and the issue subject, in the SVN commit message is not
> sufficient.
>
> Please... please... please... try to put some more meaningful context
> into commit messages.
>
> Thanks,
>
> --jason
>
>
> On Nov 27, 2006, at 2:54 AM, vamsic007@apache.org wrote:
>
> > Author: vamsic007
> > Date: Mon Nov 27 02:54:38 2006
> > New Revision: 479586
> >
> > URL: http://svn.apache.org/viewvc?view=rev&rev=479586
> > Log:
> > GERONIMO-2458 MapEditor does not work
> >
> > Modified:
> >     geronimo/server/trunk/modules/geronimo-common/src/main/java/org/
> > apache/geronimo/common/propertyeditor/MapEditor.java
> >
> > Modified: geronimo/server/trunk/modules/geronimo-common/src/main/
> > java/org/apache/geronimo/common/propertyeditor/MapEditor.java
> > URL: http://svn.apache.org/viewvc/geronimo/server/trunk/modules/
> > geronimo-common/src/main/java/org/apache/geronimo/common/
> > propertyeditor/MapEditor.java? 
> view=diff&rev=479586&r1=479585&r2=479586
> >  
> ======================================================================
> > ========
> > --- geronimo/server/trunk/modules/geronimo-common/src/main/java/org/
> > apache/geronimo/common/propertyeditor/MapEditor.java (original)
> > +++ geronimo/server/trunk/modules/geronimo-common/src/main/java/org/
> > apache/geronimo/common/propertyeditor/MapEditor.java Mon Nov 27
> > 02:54:38 2006
> > @@ -19,17 +19,22 @@
> >
> >  import java.io.ByteArrayInputStream;
> >  import java.io.IOException;
> > +import java.util.Iterator;
> >  import java.util.Properties;
> >  import java.util.Map;
> >
> > +import org.apache.commons.logging.Log ;
> > +import org.apache.commons.logging.LogFactory;
> > +
> >  /**
> > - * A property editor for {@link java.util.Properties}.
> > + * A property editor for {@link java.util.Map}.
> >   *
> >   * @version $Rev$ $Date$
> >   */
> >  public class MapEditor
> >     extends TextPropertyEditorSupport
> >  {
> > +    private static final Log log = LogFactory.getLog
> > (MapEditor.class);
> >      /**
> >       *
> >       * @throws PropertyEditorException  An IOException occured.
> > @@ -50,11 +55,30 @@
> >          Map map = (Map) getValue();
> >          if (!(map instanceof Properties)) {
> >              Properties p = new Properties();
> > -            if (map != null) {
> > -                p.putAll(map);
> > +            if(map != null) {
> > +                if(!map.containsKey(null) && !map.containsValue
> > (null)) {
> > +                    p.putAll(map);
> > +                } else {
> > +                    // Map contains null keys or values.  Replace
> > null with empty string.
> > +                    log.warn("Map contains null keys or values.
> > Replacing null values with empty string.");
> > +                    for(Iterator itr = map.entrySet().iterator();
> > itr.hasNext(); ) {
> > +                        Map.Entry entry = (Map.Entry) itr.next();
> > +                        Object key = entry.getKey();
> > +                        Object value = entry.getValue();
> > +                        if(key == null) {
> > +                            key = "";
> > +                        }
> > +                        if(value == null) {
> > +                            value = "";
> > +                        }
> > +                        p.put(key, value);
> > +                    }
> > +                }
> > +                map = p;
> >              }
> > -            map = p;
> >          }
> > -        return map.toString();
> > +        PropertiesEditor pe = new PropertiesEditor();
> > +        pe.setValue(map);
> > +        return pe.getAsText();
> >      }
> >  }
> >
> >
>
>


Mime
View raw message