Mailing-List: contact dev-help@jmeter.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@jmeter.apache.org
MIME-Version: 1.0
In-Reply-To: 
 <CAOGo0VYnHAz=Eb_O4G2wwJuQDHz6BQKj3nFD9wopNFeyY7DD-A@mail.gmail.com>
References: <20160227125318.50D953A027A@svn01-us-west.apache.org>
	<CAOGo0VY5crzLPvtBs+xqkR2Bt9KGQHGjW+Tavwxb011PNgc+6Q@mail.gmail.com>
	<CAOGo0VbD1kT5Nm6bYHfa9m_U+kZjFxGQ1K7+GOgDHWCvJGcZtw@mail.gmail.com>
	<CAH9fUpZH4ys-=Gh5bV+5JSc0hoLkHmxfzpM1BUnK+YK21spA4w@mail.gmail.com>
	<CAOGo0VZU_DedmDA1=GMpt04qeh4U5jvz_CCAAWdGFGjuKzKmWg@mail.gmail.com>
	<CAH9fUpYPuwjvO=y6bEL+++UtBVww_T91ygaD8RMkxhgWNv4tSw@mail.gmail.com>
	<CAOGo0VYnHAz=Eb_O4G2wwJuQDHz6BQKj3nFD9wopNFeyY7DD-A@mail.gmail.com>
Date: Sun, 28 Feb 2016 23:43:28 +0100
Message-ID: 
 <CAH9fUpYY-JcHjkDha+ZsoOqYDjn_iu6Bn7GzdChMrJo6QNbY1Q@mail.gmail.com>
Subject: Re: svn commit: r1732634 - in
 /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http:
 control/CookieManager.java gui/CookiePanel.java
From: Philippe Mouawad <philippe.mouawad@gmail.com>
To: "dev@jmeter.apache.org" <dev@jmeter.apache.org>
Content-Type: multipart/alternative; boundary=001a114a6a96ce1bde052cdc42a9

--001a114a6a96ce1bde052cdc42a9
Content-Type: text/plain; charset=UTF-8

On Sunday, February 28, 2016, sebb <sebbaz@gmail.com> wrote:

> On 28 February 2016 at 21:51, Philippe Mouawad
> <philippe.mouawad@gmail.com <javascript:;>> wrote:
> > Hi sebb,
> > Could you make a Patch to see what final code will look like ?
>
> Not until you revert the commit.


That's funny.

do it if you want, I have no time to lose reverting code that works and is
readable.
I don't see under what particular conditions you cannot make a patch and I
would have to revert my work


>
> > Frankly I don't understand why you want me to revert, there is no bug
> and I
> > think code is easier to read and understand (without documentation).
>
> The commit included two separate changes AND it changed constants.


No, the aim was to fix and cleanup code which was for me unreadable.

You can disagree but it does not mean you are right


>
> Please now revert the commit r1732634


 Feel free to do it.


> > What constants did I rename ? I introduced new ones.
>
> DEFAULT_POLICY => POLICY_FOR_BACKWARD_COMPATIBILITY


I then reintroduced Default_policy with the real default policy.


> > We can change their values , I cannot imagine Defaults will remain as is
> > for years.
>
> No we cannot, because that changes the way JMXs are interpreted


Not acceptable for me.
I remember we already did it in the past.
One thing I remember are the defaults of transaction controller, processing
time inclusion and generate parent sample.
At least 2 or 3 changes


>
> > We can change their values, because we created a 3.0 and can introduce
> > breaking changes, knowing that we document a lot all breaking changes (I
> > don't see many project who care so much about documenting all the
> changes).
> >
> > Regards
> > Philippe
> >
> >
> > On Sun, Feb 28, 2016 at 10:44 PM, sebb <sebbaz@gmail.com <javascript:;>>
> wrote:
> >
> >> My objection is two-fold:
> >>
> >> Public constants must not be renamed, nor (in general) can their
> >> values be changed.
> >>
> >> The fix to CookiePanel is a separate bug: i.e. the implementation must
> >> be set before the policy.
> >> It's basically a continuation of the fix I made in r1732595 (I forgot
> >> to check for other instances of the wrong ordering)
> >> It does not depend on the change to CookieManager; in fact that change
> >> does not affect it at all because DEFAULT_IMPLEMENTATION is defined in
> >> CookiePanel.
> >>
> >> So please revert the commit.
> >>
> >> You can then reapply the change to CookiePanel.
> >>
> >> I will then update CookieManager to better document the constant.
> >>
> >>
> >> On 27 February 2016 at 13:58, Philippe Mouawad
> >> <philippe.mouawad@gmail.com <javascript:;>> wrote:
> >> > On Sat, Feb 27, 2016 at 2:52 PM, sebb <sebbaz@gmail.com
> <javascript:;>> wrote:
> >> >
> >> >> Also the commit fixed a bug and made some unrelated changes.
> >> >>
> >> >> Don't mix separate fixes.
> >> >>
> >> >> It's much harder to review and to follow the history later.
> >> >>
> >> >> Please revert and re-apply the change to CookiePanel separately.
> >> >>
> >> > It is part of the fix.
> >> >
> >> >>
> >> >> I don't agree with the change to CookieManager; that needs further
> >> >> discussion.
> >> >>
> >> >
> >> > Based  on my last commit few minutes ago, can you open a new
> discussion
> >> (as
> >> > subject is not clear) to mention what you disagree with ?
> >> > Thanks
> >> >
> >> >>
> >> >>
> >> >> On 27 February 2016 at 13:45, sebb <sebbaz@gmail.com <javascript:;>>
> wrote:
> >> >> > -1
> >> >> >
> >> >> > Must not change public constant names.
> >> >> >
> >> >> > If the name is not ideal, fix this by adding Javadoc, not a rename.
> >> >> >
> >> >> > On 27 February 2016 at 12:53,  <pmouawad@apache.org <javascript:;>>
> wrote:
> >> >> >> Author: pmouawad
> >> >> >> Date: Sat Feb 27 12:53:18 2016
> >> >> >> New Revision: 1732634
> >> >> >>
> >> >> >> URL: http://svn.apache.org/viewvc?rev=1732634&view=rev
> >> >> >> Log:
> >> >> >> Bug 58756 - CookieManager : Cookie Policy select box content must
> >> >> depend on Cookie implementation
> >> >> >> A) Fix bug introduced by commit r1732632:
> >> >> >> 1/ Save with 2.13 a Plan containing CookieManager that uses
> defaults
> >> >> >> 2/ Open it with trunk
> >> >> >> 3/ It is ok
> >> >> >> 4/ Close it
> >> >> >> 5/ Add a new CookieManager => KO the policy is default instead of
> >> >> standard
> >> >> >>
> >> >> >>
> >> >> >> B) Also made constants explicit and removed dependency on
> deprecated
> >> >> class.
> >> >> >>
> >> >> >> C) Forced the save of policy and implementation even if there
> values
> >> >> are set at defaults to avoid this headache again
> >> >> >>
> >> >> >>
> >> >> >> Bugzilla Id: 58756
> >> >> >>
> >> >> >> Modified:
> >> >> >>
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
> >> >> >>
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java
> >> >> >>
> >> >> >> Modified:
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
> >> >> >> URL:
> >> >>
> >>
> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java?rev=1732634&r1=1732633&r2=1732634&view=diff
> >> >> >>
> >> >>
> >>
> ==============================================================================
> >> >> >> ---
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
> >> >> (original)
> >> >> >> +++
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
> >> >> Sat Feb 27 12:53:18 2016
> >> >> >> @@ -30,7 +30,6 @@ import java.io.Serializable;
> >> >> >>  import java.net.URL;
> >> >> >>  import java.util.ArrayList;
> >> >> >>
> >> >> >> -import org.apache.http.client.config.CookieSpecs;
> >> >> >>  import org.apache.jmeter.config.ConfigTestElement;
> >> >> >>  import org.apache.jmeter.engine.event.LoopIterationEvent;
> >> >> >>  import org.apache.jmeter.testelement.TestIterationListener;
> >> >> >> @@ -102,12 +101,19 @@ public class CookieManager extends Confi
> >> >> >>
> >> >> >>      private transient CollectionProperty initialCookies;
> >> >> >>
> >> >> >> -    // MUST NOT BE CHANGED
> >> >> >> -    @SuppressWarnings("deprecation") // cannot be changed
> >> >> >> -    public static final String DEFAULT_POLICY =
> >> >> CookieSpecs.BROWSER_COMPATIBILITY;
> >> >> >> +    // MUST NOT BE CHANGED because as defaults are not saved,
> >> >> >> +    // when a Test Plan was loaded from an N-1 version and if
> >> defaults
> >> >> changed,
> >> >> >> +    // you end up changing the previously set policy
> >> >> >> +    // see issues with Bug 58756
> >> >> >> +    public static final String POLICY_FOR_BACKWARD_COMPATIBILITY
> =
> >> >> "compatibility";
> >> >> >>
> >> >> >> -    // MUST NOT BE CHANGED
> >> >> >> -    public static final String DEFAULT_IMPLEMENTATION =
> >> >> HC3CookieHandler.class.getName();
> >> >> >> +    // MUST NOT BE CHANGED because as defaults are not saved,
> >> >> >> +    // when a Test Plan was loaded from an N-1 version and if
> >> defaults
> >> >> changed,
> >> >> >> +    // you end up changing the previously set policy
> >> >> >> +    // see issues with Bug 58756
> >> >> >> +    public static final String
> >> >> IMPLEMENTATION_FOR_BACKWARD_COMPATIBILITY =
> >> >> HC3CookieHandler.class.getName();
> >> >> >> +
> >> >> >> +    public static final String DEFAULT_IMPLEMENTATION =
> >> >> HC4CookieHandler.class.getName();
> >> >> >>
> >> >> >>      public CookieManager() {
> >> >> >>          clearCookies(); // Ensure that there is always a
> collection
> >> >> available
> >> >> >> @@ -124,11 +130,13 @@ public class CookieManager extends Confi
> >> >> >>      }
> >> >> >>
> >> >> >>      public String getPolicy() {
> >> >> >> -        return getPropertyAsString(POLICY, DEFAULT_POLICY);
> >> >> >> +        return getPropertyAsString(POLICY,
> >> >> POLICY_FOR_BACKWARD_COMPATIBILITY);
> >> >> >>      }
> >> >> >>
> >> >> >>      public void setCookiePolicy(String policy){
> >> >> >> -        setProperty(POLICY, policy, DEFAULT_POLICY);
> >> >> >> +        // we must explicitely save the policy
> >> >> >> +        // not use a default implementation
> >> >> >> +        setProperty(POLICY, policy);
> >> >> >>      }
> >> >> >>
> >> >> >>      public CollectionProperty getCookies() {
> >> >> >> @@ -148,11 +156,13 @@ public class CookieManager extends Confi
> >> >> >>      }
> >> >> >>
> >> >> >>      public String getImplementation() {
> >> >> >> -        return getPropertyAsString(IMPLEMENTATION,
> >> >> DEFAULT_IMPLEMENTATION);
> >> >> >> +        return getPropertyAsString(IMPLEMENTATION,
> >> >> IMPLEMENTATION_FOR_BACKWARD_COMPATIBILITY);
> >> >> >>      }
> >> >> >>
> >> >> >>      public void setImplementation(String implementation){
> >> >> >> -        setProperty(IMPLEMENTATION, implementation,
> >> >> DEFAULT_IMPLEMENTATION);
> >> >> >> +        // we must explicitely save the policy
> >> >> >> +        // not use a default implementation
> >> >> >> +        setProperty(IMPLEMENTATION, implementation);
> >> >> >>      }
> >> >> >>
> >> >> >>      /**
> >> >> >>
> >> >> >> Modified:
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java
> >> >> >> URL:
> >> >>
> >>
> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java?rev=1732634&r1=1732633&r2=1732634&view=diff
> >> >> >>
> >> >>
> >>
> ==============================================================================
> >> >> >> ---
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java
> >> >> (original)
> >> >> >> +++
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java
> >> >> Sat Feb 27 12:53:18 2016
> >> >> >> @@ -280,9 +280,9 @@ public class CookiePanel extends Abstrac
> >> >> >>
> >> >> >>          tableModel.clearData();
> >> >> >>          clearEachIteration.setSelected(false);
> >> >> >> -        policy.setText(HC4CookieHandler.DEFAULT_POLICY_NAME);
> >> >> >>          selectHandlerPanel.setSelectedItem(DEFAULT_IMPLEMENTATION
> >> >> >>
> .substring(DEFAULT_IMPLEMENTATION.lastIndexOf('.') +
> >> >> 1));
> >> >> >> +        policy.setText(HC4CookieHandler.DEFAULT_POLICY_NAME);
> >> >> >>          deleteButton.setEnabled(false);
> >> >> >>          saveButton.setEnabled(false);
> >> >> >>      }
> >> >> >>
> >> >> >>
> >> >>
> >> >
> >> >
> >> >
> >> > --
> >> > Cordialement.
> >> > Philippe Mouawad.
> >>
> >
> >
> >
> > --
> > Cordialement.
> > Philippe Mouawad.
>


-- 
Cordialement.
Philippe Mouawad.

--001a114a6a96ce1bde052cdc42a9--