On Tuesday, March 1, 2016, sebb <[email protected]> wrote: > On 29 February 2016 at 17:17, Philippe Mouawad > <[email protected] <javascript:;>> wrote: > > Hi Sebb > > I hope my new answers will be clear. > > > > Regards > > Philippe > > > > On Mon, Feb 29, 2016 at 4:25 PM, sebb <[email protected] <javascript:;> > > <javascript:_e(%7B%7D,'cvml','[email protected] <javascript:;>');>> > wrote: > > > >> On 29 February 2016 at 12:27, Philippe Mouawad > >> <[email protected] <javascript:;> > >> <javascript:_e(%7B%7D,'cvml','[email protected] > >> <javascript:;>');>> > wrote: > >> > Greetings sebb, > >> > > >> > I reviewed the code commited in trunk and sorry, I don't find it clear > >> and > >> > it does not suit me. > >> > >> Have you reviewed the patch? > >> > > > > Absolutely. I read it 10 times, the mail + the commit (the reverts one my > > code). > > > > > >> > >> Trunk does not have the descriptive comments yet. > >> > > > > Ok but I don't think I wrote anything about that > > You wrote: > > > I reviewed the code commited in trunk and sorry, I don't find it clear > and > > it does not suit me. > > You made no comment about my patch, so I had to assume you had not read it.
you were wrong > > >> > >> > For me it is an issue that Default policy ( > >> > > >> > https://github.com/apache/jmeter/blob/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java#L107 > >> ) > >> > and implementation ( > >> > > >> > https://github.com/apache/jmeter/blob/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java#L110 > >> ) > >> > in CookieManager are not the real defaults and that the GUI holds the > >> real > >> > defaults ( > >> > > >> > https://github.com/apache/jmeter/blob/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java#L90 > >> , > >> > > >> > https://github.com/apache/jmeter/blob/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java#L95 > >> ), > >> > for me: > >> > > >> > - it makes code "ugly", a field named DEFAULT_POLICY in the model > is > >> NOT > >> > the default policy ?, same for DEFAULT_IMPLEMENTATION ? really > strange > >> > >> Have you read the patch which clearly documents what the fields mean? > >> > > > > Absolutely, and as I wrote, my problem is not the documentation, it is > the > > code, I think good code should be a kind of "Auto documented". Of course > > javadocs and docs are welcome but they don't fix "bad" code. > > I see that you deprecate it, but for me it should remain here and be > > changed to reflect the new default. > > Public constants cannot be changed. Do as you like. it seems you don't want to take into account any of my arguments ... > > I agree that using a public constant for these values was a mistake, > but it's too late now. > > The code is working now, and AFAICT the only problem you have is that > the constant has the wrong name. it did with my code > > I'm sorry, but you will just have to live with it. that's argumentation. Because you're the final decider ? ... > > > > >> > - it breaks MVC pattern as it's the GUI which overrides the Model > >> > >> It *IS* the GUI which applies the defaults. > >> > > > > This is exactly my problem. It breaks for me the MVC model, as the Model > is > > not the GUI, so it should be the sampler that decides about the default. > > But ALL the test elements work the same way. > The clear() method is what initialises the GUI, and that is in the GUI. No, check the code > > It may well be that they don't fit the MVC model. > The design has been around for a very long time, probably before the > model existed. > I don't think we should rewrite the whole GUI just to fit in with one > particular model. let's not change anything , never ever... > > > Another option is to make it private. > > How many times do I have to say this: Repeating something wrong do not maje it right. > > Public constants cannot be changed. > > > > > > >> This is only exposed here because the GUI default has been changed > >> from the JMX default. > >> > > > > I am aware of that. > > > >> > >> > So -1 for me. > >> > >> We cannot change the public DEFAULT constant names or values in > >> CookieManager without breaking compatibility. > >> > > > > I disagree with this note. > > My previous patch did not break any backward compatibility except for 3rd > > party code using the Java Constants. > > As such , as we switch to 3.0 we CAN break this APIs as we WILL document > in > > changes that we changed this. > > Backward compatibility is important, but it can't be the sole MOTTO of > > JMeter. I would like this release to drop all the "concessions" to > Backward > > compatibility that were made (and were good at the time they were made), > > but we cannot live indefinitely with them, let 3.0 be the rocket that > will > > drive us to the Mars of Load Testing tools... > > That is a meaningless statement. Just some joke ... No need to be rude > > > > > That's also why I proposed to drop for future evolutions the "pattern" > that > > we used until now which was : > > - setProperty(key, value, default_value ) => Which results in default not > > being written to JMX. It that had been done since the begining, we would > be > > able to change the default without any problem, the bug would not have > > triggered > > Using a default does NOT cause a bug. It does > > > The problem is using the same constant for the JMX default as for the > GUI default. > > Which I solved by creating a new constant in the GUI class. > > However I will happily move the constant to the sampler class if that > will help make it look more like an MVC model. As you like. I am tired of this discussion > > > By only calling setProperty(key, value) , the default will be written to > > file and that's it. > > The impact on jmx will be a slight increase in size. > > Yes it might break some test cases, but we can fix them. > > But there is no need to do this. > > What we need to do going forward is ensure that the JMX and GUI > default constants are clearly documented. let's document bad design. > > Both constants should ideally be private, with a getter for the GUI > default if necessary. > > If the constant is private, of course it can be renamed. > However its value must remain. > As you like -- Cordialement. Philippe Mouawad.
