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 Mon, 27 Nov 2006 21:28:42 GMT
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