geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Bohn <joe.b...@earthlink.net>
Subject Re: svn commit: r479586 - /geronimo/server/trunk/modules/geronimo-common/src/main/java/org/apache/geronimo/common/propertyeditor/MapEditor.java
Date Mon, 27 Nov 2006 21:52:44 GMT

I agree with Jason that the change log should include a list of changes 
rather than problem statements.  After all, it is a *change* log and not 
a *problem* log. :-)  Just a brief summary of the change is all that's 
needed and I personally find this much more helpful than a one line 
description of a problem taken from the JIRA.  Of course, the 
description should also include a reference to the JIRA.

I also agree with Donald (and I suspect Jason does too) that the JIRA is 
the right place for all the gory details on the problem and solution.

So, can we agree to keep the details in the JIRA but have a brief 
descriptive statement of what changed in the commit log rather than a 
statement of the problem?

Joe


Jason Dillon wrote:
> I completely disagree.  The change log should contain context of the  
> change, and not just the link to JIRA.
> 
> --jason
> 
> 
> On Nov 27, 2006, at 12:56 PM, Donald Woods wrote:
> 
>> Providing a link to the JIRA makes more sense, as it can contain  the 
>> full description of the fix, which releases it went in, any  
>> coreq/prereqs like OpenEJB or TranQL changes, if multiple patches  
>> and/or commits were required, .....
>>
>> As long as everyone adds real problem and solution descriptions in  
>> the JIRAs, I'm fine with only providing a link back to it in a  
>> commit, which lots of people before Vamsi have done in the past  
>> without complaint :-)
>>
>>
>> -Donald
>>
>>
>> Jason Dillon wrote:
>>
>>> As I mentioned... I really do not want to have to bounce from  commit 
>>> messages to JIRA to provide oversight into the changes  going into 
>>> our SVN repo.
>>> Also, when something does break, I am going to want to look back  
>>> through the SVN logs and find meaningful context to changes.  In  
>>> this case "GERONIMO-2458 MapEditor does not work" does not tell me  
>>> what was changed, only tells me what was wrong.
>>> IMO SVN is the definitive location for changes... and those  changes 
>>> should be properly adorned with context of the change.
>>> --jason
>>> On Nov 27, 2006, at 12:08 PM, Donald Woods wrote:
>>>
>>>> The problem description and fix details were included in the JIRA  
>>>> by Vamsi as they should be - http://issues.apache.org/jira/browse/ 
>>>> GERONIMO-2458
>>>>
>>>>
>>>> -Donald
>>>>
>>>>
>>>> Jason Dillon 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