Mailing-List: contact cxf-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: cxf-dev@incubator.apache.org
Received-SPF: pass (nike.apache.org: domain of jim.ma@iona.com designates
 12.170.54.180 as permitted sender)
Message-ID: <46EDE58C.60906@iona.com>
Date: Mon, 17 Sep 2007 10:25:16 +0800
From: Jim Ma <ema@iona.com>
User-Agent: Thunderbird 2.0.0.6 (Windows/20070728)
MIME-Version: 1.0
To: cxf-dev@incubator.apache.org
Subject: Re: svn commit: r575222 - in
 /incubator/cxf/trunk:	api/src/main/java/org/apache/cxf/tools/common/
 tools/common/	tools/common/src/main/java/org/apache/cxf/tools/common/
 tools/javato/
 tools/javato/ws/	tools/javato/ws/src/main/java/org/apache/cxf/tools/java2w...
References: <20070913084524.252BF1A9844@eris.apache.org>
 <1189796159.26708.25.camel@gmazza-desktop>
In-Reply-To: <1189796159.26708.25.camel@gmazza-desktop>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Glen ,

Please see my comments inline.

Glen Mazza wrote:
> Am Donnerstag, den 13.09.2007, 08:45 +0000 schrieb ema@apache.org:
>
>   
>> Modified: incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2ws/JavaToWSContainer.java
>> URL: http://svn.apache.org/viewvc/incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2ws/JavaToWSContainer.java?rev=575222&r1=575221&r2=575222&view=diff
>> ==============================================================================
>> --- incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2ws/JavaToWSContainer.java (original)
>> +++ incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2ws/JavaToWSContainer.java Thu Sep 13 01:45:21 2007
>> @@ -78,7 +78,7 @@
>>
>>              if (!"simple".equalsIgnoreCase(ft) && !"jaxws".equalsIgnoreCase(ft)) {
>> -                Message msg = new Message("INVALID_FORNTEND", LOG, new Object[]{ft});               
>> +                Message msg = new Message("INVALID_FRONTEND", LOG, new Object[] {ft});
>>                  errs.add(new ErrorVisitor.UserError(msg.toString()));
>>              }
>>          }
>>
>>     
> I think we should make the constants--"jaxws" and
> "simple"--case-sensitive, it will help us in the future should we use
> constants for these two values.  Also, our command-line scripts are
> already case-sensitive (correct?), and Unix-like case sensitivity gives
> a good feeling to the user that the scripts are very rigorous and
> robust.
>
>   
+1.

>   
>> +                Message msg = new Message("WRAPPERBEAN_WITHOUT_JAXWS", LOG);
>>                  errs.add(new ErrorVisitor.UserError(msg.toString()));
>>              }
>>          }
>> -  
>>  
>>
>> Modified: incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2ws/Messages.properties
>> URL: http://svn.apache.org/viewvc/incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2ws/Messages.properties?rev=575222&r1=575221&r2=575222&view=diff
>> ==============================================================================
>> --- incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2ws/Messages.properties (original)
>> +++ incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2ws/Messages.properties Thu Sep 13 01:45:21 2007
>> @@ -23,6 +23,7 @@
>>  FILE_NOT_EXIST = File does not exist
>>  NOT_A_FILE = {0} is not a file
>>  PARAMETER_MISSING = Required parameter is missing or not valid
>> -INVALID_FORNTEND = \ "{0}" is not a valid frontend, java2ws only supports jaxws and simple frontend.
>> -CANT_GEN_WRAPPERBEAN = Wrapperbean only needs to be generated for jaxws front end.
>> +INVALID_FRONTEND = "{0}" is not a valid frontend, java2ws only supports jaxws and the simple frontend.
>>     
>
> INVALID_FRONTEND = "{0}" is not a valid frontend, java2ws only supports
> the "simple" and "jaxws" frontend.
>
> We need quotes so the reader knows what to type in; also, if we put
> simple last it seems to be a sarcastic comment (i.e., "simple" frontend
> can mean not-very-simple front end)
>
>   
+1. I think it is a good practice to quote the reader need to type in .
>> +WRAPPERBEAN_WITHOUT_JAXWS = -wrapperbean is only valid for the jaxws front end.
>> +INVALID_DATABINDING = Invalid value {0} for data binding type. 
>>  
>>
>> Modified: incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2ws/java2ws.xml
>> URL: http://svn.apache.org/viewvc/incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2ws/java2ws.xml?rev=575222&r1=575221&r2=575222&view=diff
>> ==============================================================================
>> --- incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2ws/java2ws.xml (original)
>> +++ incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2ws/java2ws.xml Thu Sep 13 01:45:21 2007
>> @@ -35,9 +35,19 @@
>>  	</annotation>
>>  	<usage>
>>  		<optionGroup id="options">
>> +		
>> +			<option id="databinding" maxOccurs="1">
>> +				<annotation>
>> +				    Specify the data binding (aegis or jaxb). Default jaxb.
>> +				</annotation>
>> +				<switch>databinding</switch>
>> +				<associatedArgument placement="afterSpace">
>> +				  <annotation>jaxb or aegis</annotation>
>> +				</associatedArgument>
>> +			</option>
>>  			<option id="frontend" maxOccurs="1">
>>  				<annotation>
>> -					specify which frontend should be use, support jaxws and simple frontend.
>> +					Specify which frontend should be use, support jaxws and simple frontend.
>>     
>
> Specify the frontend to use, either "simple" or "jaxws" frontend.
>
>
>   
>>  				</annotation>
>>  				<switch>frontend</switch>
>>  				<associatedArgument placement="afterSpace">
>>
>>
>> Modified:
>> incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2wsdl/processor/internal/ServiceBuilderFactory.java
>> URL:
>> http://svn.apache.org/viewvc/incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2wsdl/processor/internal/ServiceBuilderFactory.java?rev=575222&r1=575221&r2=575222&view=diff
>> ==============================================================================
>> ---
>> incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2wsdl/processor/internal/ServiceBuilderFactory.java (original)
>> +++
>> incubator/cxf/trunk/tools/javato/ws/src/main/java/org/apache/cxf/tools/java2wsdl/processor/internal/ServiceBuilderFactory.java Thu Sep 13 01:45:21 2007
>> @@ -19,17 +19,28 @@
>>  
>>  
>> -    public ServiceBuilder newBuilder(FrontendFactory.Style s) {
>> +        String beanName = getBuilderBeanName(s);
>>          ServiceBuilder builder = null;
>> +
>>          try {
>> -            String clzName = getBuilderClassName(s);
>> -            builder = (ServiceBuilder)
>> Class.forName(clzName).newInstance();
>> -        } catch (Exception e) {
>> -            throw new ToolException("Can not find or initialize the
>> ServiceBuilder for style: " + s
>> +            builder = (ServiceBuilder)
>> applicationContext.getBean(beanName, ServiceBuilder.class);
>> +            AbstractServiceFactory serviceFactory =
>> (AbstractServiceFactory)builder;
>> +            serviceFactory.setDataBinding(dataBinding);
>> +        } catch (RuntimeException e) {
>> +            throw new ToolException("Can not get ServiceBuilder bean
>> " + beanName 
>> +                                    + "to initialize the
>> ServiceBuilder for style: " + s
>>     
>
> " to initialize..." (leading space needed)
>
>
>   
>> Added: incubator/cxf/trunk/tools/javato/ws/src/test/java/org/apache/cxf/tools/fortest/aegis2ws/Something.java
>> URL: http://svn.apache.org/viewvc/incubator/cxf/trunk/tools/javato/ws/src/test/java/org/apache/cxf/tools/fortest/aegis2ws/Something.java?rev=575222&view=auto
>> ==============================================================================
>> --- incubator/cxf/trunk/tools/javato/ws/src/test/java/org/apache/cxf/tools/fortest/aegis2ws/Something.java (added)
>> +++ incubator/cxf/trunk/tools/javato/ws/src/test/java/org/apache/cxf/tools/fortest/aegis2ws/Something.java Thu Sep 13 01:45:21 2007
>> @@ -0,0 +1,52 @@
>> +/**
>> + * Licensed to the Apache Software Foundation (ASF) under one
>> + * or more contributor license agreements. See the NOTICE file
>> + * distributed with this work for additional information
>> + * regarding copyright ownership. The ASF licenses this file
>> + * to you 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
>> + *
>> + * http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing,
>> + * software distributed under the License is distributed on an
>> + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
>> + * KIND, either express or implied. See the License for the
>> + * specific language governing permissions and limitations
>> + * under the License.
>> + */
>> +package org.apache.cxf.tools.fortest.aegis2ws;
>> +
>> +/**
>> + * Test data type for Aegis in java2ws
>> + */
>> +public class Something {
>> +    private String multiple;
>> +    private String singular;
>> +    
>>     
>
> Is there any semantic meaning for multiple and singular?  The reader
> gets that impression, but does know what those words mean.  If there
> *is* a meaning, a quick comment in this file of what a single and a
> multiple would be good.  If there are *no* meanings, usually "foo" and
> "bar" are better to indicate that.
>
>
>   
This class is for test case and come from Benson's patch  . I think it 
is good to change it to "bar" and "foo".
Benson , do you think so ?


Thanks

Jim