Hello Sebb,
Sorry for this:

   - Agree with equals instead of start
   - For formatting as my eclipse was configured for ignoring whitespaces I
   didn't see all these formatting issues.



Regards
Philippe

On Fri, Oct 7, 2011 at 2:43 PM, sebb <seb...@gmail.com> wrote:

> On 7 October 2011 13:01, sebb <seb...@gmail.com> wrote:
> > On 7 October 2011 12:51, sebb <seb...@gmail.com> wrote:
> >> On 7 October 2011 12:10,  <pmoua...@apache.org> wrote:
> >>> Author: pmouawad
> >>> Date: Fri Oct  7 11:10:18 2011
> >>> New Revision: 1180004
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=1180004&view=rev
> >>> Log:
> >>> Bug 51988 - CSV Data Set Configuration does not resolve default
> delimiter for header parsing when variables field is empty
> >>>
> >>> Modified:
> >>>
>  jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java
> >>>    jakarta/jmeter/trunk/xdocs/changes.xml
> >>>
> >>> Modified:
> jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java
> >>> URL:
> http://svn.apache.org/viewvc/jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java?rev=1180004&r1=1180003&r2=1180004&view=diff
> >>>
> ==============================================================================
> >>> ---
> jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java
> (original)
> >>> +++
> jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java
> Fri Oct  7 11:10:18 2011
> >>> @@ -100,74 +100,74 @@ public class CSVDataSet extends ConfigTe
> >>>     public void iterationStart(LoopIterationEvent iterEvent) {
> >>>         FileServer server = FileServer.getFileServer();
> >>>         final JMeterContext context = getThreadContext();
> >>> +        String delim = getDelimiter();
> >>> +        if (delim.startsWith("\\t")) { // $NON-NLS-1$
> >>
> >> This is a change from the previous code, which uses .equals().
> >> I don't think startsWith() is suitable here.
> >>
> >>> +            delim = "\t";// Make it easier to enter a Tab //
> $NON-NLS-1$
> >>> +        } else if (delim.length() == 0) {
> >>> +            log.warn("Empty delimiter converted to ','");
> >>> +            delim = ",";
> >>> +        }
> >>>         if (vars == null) {
> >>>             String _fileName = getFilename();
> >>>             String mode = getShareMode();
> >>>             int modeInt = CSVDataSetBeanInfo.getShareModeAsInt(mode);
> >>> -            switch(modeInt){
> >>> -                case CSVDataSetBeanInfo.SHARE_ALL:
> >>> -                    alias = _fileName;
> >>> -                    break;
> >>> -                case CSVDataSetBeanInfo.SHARE_GROUP:
> >>> -                    alias =
> _fileName+"@"+System.identityHashCode(context.getThreadGroup());
> >>> -                    break;
> >>> -                case CSVDataSetBeanInfo.SHARE_THREAD:
> >>> -                    alias =
> _fileName+"@"+System.identityHashCode(context.getThread());
> >>> -                    break;
> >>> -                default:
> >>> -                    alias = _fileName+"@"+mode; // user-specified key
> >>> -                    break;
> >>> +            switch (modeInt) {
> >>> +            case CSVDataSetBeanInfo.SHARE_ALL:
> >>> +                alias = _fileName;
> >>> +                break;
> >>> +            case CSVDataSetBeanInfo.SHARE_GROUP:
> >>> +                alias = _fileName + "@" +
> System.identityHashCode(context.getThreadGroup());
> >>> +                break;
> >>> +            case CSVDataSetBeanInfo.SHARE_THREAD:
> >>> +                alias = _fileName + "@" +
> System.identityHashCode(context.getThread());
> >>> +                break;
> >>> +            default:
> >>> +                alias = _fileName + "@" + mode; // user-specified key
> >>> +                break;
> >>>             }
> >>>             final String names = getVariableNames();
> >>> -            if (names == null || names.length()==0) {
> >>> +            if (names == null || names.length() == 0) {
> >
> > Also, please don't change formatting when fixing bugs, unless directly
> > related - e.g. adding surrounding try/catch or if statement.
> >
> > It makes it much harder to see what has actually changed.
>
> I now see that the additional changes were in the original patch.
>
> If a submiited patch includes unnecessary changes, we have a choice:
> - either we reject it, and ask the submitter to provide a new patch
> - or we apply it, but not the spurious changes.
> This is reasonably easy to do in Eclipse, but can be tedious.
> Apply the patch, then do a compare against the base revision.
> Revert the changes that aren't relevant.
> Just before commiting, recheck changes against the server version.
>
> Either way, commits should be the minimum necessary to solve the issue.
>
> >>>                 String header = server.reserveFile(_fileName,
> getFileEncoding(), alias, true);
> >>>                 try {
> >>> -                    vars = CSVSaveService.csvSplitString(header,
> getDelimiter().charAt(0));
> >>> +                    vars = CSVSaveService.csvSplitString(header,
> delim.charAt(0));
> >>>                     firstLineIsNames = true;
> >>>                 } catch (IOException e) {
> >>> -                    log.warn("Could not split CSV header line",e);
> >>> +                    log.warn("Could not split CSV header line", e);
> >>>                 }
> >>>             } else {
> >>>                 server.reserveFile(_fileName, getFileEncoding(),
> alias);
> >>>                 vars = JOrphanUtils.split(names, ","); // $NON-NLS-1$
> >>>             }
> >>>         }
> >>> -            String delim = getDelimiter();
> >>> -            if (delim.equals("\\t")) { // $NON-NLS-1$
> >>> -                delim = "\t";// Make it easier to enter a Tab //
> $NON-NLS-1$
> >>> -            } else if (delim.length()==0){
> >>> -                log.warn("Empty delimiter converted to ','");
> >>> -                delim=",";
> >>> -            }
> >>> -            // TODO: fetch this once as per vars above?
> >>> -            JMeterVariables threadVars = context.getVariables();
> >>> -            String line = null;
> >>> +        // TODO: fetch this once as per vars above?
> >>> +        JMeterVariables threadVars = context.getVariables();
> >>> +        String line = null;
> >>> +        try {
> >>> +            line = server.readLine(alias, getRecycle(),
> firstLineIsNames);
> >>> +        } catch (IOException e) { // treat the same as EOF
> >>> +            log.error(e.toString());
> >>> +        }
> >>> +        if (line != null) {// i.e. not EOF
> >>>             try {
> >>> -                line = server.readLine(alias, getRecycle(),
> firstLineIsNames);
> >>> -            } catch (IOException e) { // treat the same as EOF
> >>> -                log.error(e.toString());
> >>> -            }
> >>> -            if (line!=null) {// i.e. not EOF
> >>> -                try {
> >>> -                    String[] lineValues = getQuotedData() ?
> >>> -                            CSVSaveService.csvSplitString(line,
> delim.charAt(0))
> >>> -                            : JOrphanUtils.split(line, delim, false);
> >>> -                            for (int a = 0; a < vars.length && a <
> lineValues.length; a++) {
> >>> -                                threadVars.put(vars[a],
> lineValues[a]);
> >>> -                            }
> >>> -                } catch (IOException e) { // Should only happen for
> quoting errors
> >>> -                   log.error("Unexpected error splitting '"+line+"' on
> '"+delim.charAt(0)+"'");
> >>> -                }
> >>> -                // TODO - report unused columns?
> >>> -                // TODO - provide option to set unused variables ?
> >>> -            } else {
> >>> -                if (getStopThread()) {
> >>> -                    throw new JMeterStopThreadException("End of file
> detected");
> >>> -                }
> >>> -                for (int a = 0; a < vars.length ; a++) {
> >>> -                    threadVars.put(vars[a], EOFVALUE);
> >>> +                String[] lineValues = getQuotedData() ?
> >>> +                        CSVSaveService.csvSplitString(line,
> delim.charAt(0))
> >>> +                        : JOrphanUtils.split(line, delim, false);
> >>> +                for (int a = 0; a < vars.length && a <
> lineValues.length; a++) {
> >>> +                    threadVars.put(vars[a], lineValues[a]);
> >>>                 }
> >>> +            } catch (IOException e) { // Should only happen for
> quoting errors
> >>> +                log.error("Unexpected error splitting '" + line + "'
> on '" + delim.charAt(0) + "'");
> >>> +            }
> >>> +            // TODO - report unused columns?
> >>> +            // TODO - provide option to set unused variables ?
> >>> +        } else {
> >>> +            if (getStopThread()) {
> >>> +                throw new JMeterStopThreadException("End of file
> detected");
> >>>             }
> >>> +            for (int a = 0; a < vars.length ; a++) {
> >>> +                threadVars.put(vars[a], EOFVALUE);
> >>> +            }
> >>> +        }
> >>>     }
> >>>
> >>>     /**
> >>>
> >>> Modified: jakarta/jmeter/trunk/xdocs/changes.xml
> >>> URL:
> http://svn.apache.org/viewvc/jakarta/jmeter/trunk/xdocs/changes.xml?rev=1180004&r1=1180003&r2=1180004&view=diff
> >>>
> ==============================================================================
> >>> --- jakarta/jmeter/trunk/xdocs/changes.xml (original)
> >>> +++ jakarta/jmeter/trunk/xdocs/changes.xml Fri Oct  7 11:10:18 2011
> >>> @@ -119,6 +119,7 @@ Mirror server now uses default port 8081
> >>>  <h3>General</h3>
> >>>  <ul>
> >>>  <li>Bug 51937 - JMeter does not handle missing TestPlan entry
> well</li>
> >>> +<li>Bug 51988 - CSV Data Set Configuration does not resolve default
> delimiter for header parsing when variables field is empty</li>
> >>>  </ul>
> >>>
> >>>  <!-- =================== Improvements =================== -->
> >>>
> >>>
> >>>
> >>> ---------------------------------------------------------------------
> >>> To unsubscribe, e-mail: notifications-unsubscr...@jakarta.apache.org
> >>> For additional commands, e-mail: notifications-h...@jakarta.apache.org
> >>>
> >>>
> >>
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@jakarta.apache.org
> For additional commands, e-mail: dev-h...@jakarta.apache.org
>
>


-- 
Cordialement.
Philippe Mouawad.

Reply via email to