Re: [TWiki-Dev] [SVN] r3661 - in twiki/branches/DEVELOP: data/TWiki data/TestCases lib lib/TWiki test tools

[Peter T. <pet...@at...>]

Hi Crawford,

first of all let me apologize that I was shooting before asking.
I was incorrecty assuming that you added TWiki::Contrib::Attrs
which has compatibility issues with the current parameter parsing.
We had this discussion before, you have been pushing for that
module while I was pushing back because of compatibility issues.

Having said that, the new parsing rule still looks quite different 
to the current TWiki::extractParameters spec. 

I will have to check our TWiki applications to see some odd ends 
of using the current spec. I know that creating good test cases 
is a good investment in time, but it does take time...

Lets work on test cases that foremost validate the 01 Sep 2005 spec, 
_then_ refactor code and verify if it does not break the old spec. 
It is important not to define test cases that just verify refactored 
code.

On top of my head, there is one incompatibility by just reading
the code: Picture a TWiki app that gets the default parameter
from a URL parameter, as in:

%SPECIAL_APP{ "%URLPARAM{user_input}%" additional="params" are="OK" }%

The string passed to the parameter parser might look like this:

"User says "hi" to Bob" additional="params" are="OK"

TWiki::extractParameters returns a _DEFAULT of:

User says "hi" to Bob

This example is taken from an actual TWiki application. (Yes, I know
that the parser will fail if the users enters an unlikely: 
User says foo="hi" to Bob)

From reading the code it does not appear that TWiki::Contrib::Attrs 
returns the same. So if you want to introduce the extra module, why
not simply copy the code of TWiki::extractParameters to avoid 
potential compatibility issues?

On performance, each new module we add to the core TWiki code adds 
a small IO based delay. TWiki::extractParameters was part of
TWiki.pm. Now we have a new module to compile and a new object to
initialize with each parameter to parse.

To work smoother in the future, could you do one check-in per
feature / code refactor? Preceeding that, with a Codev topic that
alerts people of a change proposal ahead of time?

Kind regards,
Peter


Crawford Currie wrote:
> 
> Hi Peter,
> 
> On Tuesday 22 February 2005 06:08, Peter Thoeny wrote:
> > As you might recall I have strong reservations to replace
> > TWiki::extractParameters( $args ) with TWiki::Attrs( ) since
> > the parsing of parameters changes.
> 
> No, the parsing of parameters does not change. I revised the Attrs
> module to address the concerns I knew you would have before I put
> it in.
>    1. The extended syntax is a strict superset of the current syntax
>    2. The module can operate in two modes; compatibility mode
>        (which is meant to make it work identically to the old
>         extractParameters) and extended mode.
> 
> > This change will break a number of TWikiApplications.
> 
> Not at all. As I said, I have been careful to ensure that compatibility is
> retained. As usual with my code I have added a range of unit tests that
> I believe verify the compatibility with the existing spec, as far as I was
> able to reverse-engineer it. As usual, if you can provide a testcase that
> demonstrates a misinterpretation of the spec, I will happily add it to the
> test set.
> 
> >  It seems you prefer nice OO code over compatibility?
> 
> Given the huge amount of effort I have put in to ensure compatibility
> with existing data, I feel this remark is grossly unfair. I have developed
> a wide range of testcases that demonstrate what the code does, and
> have designed those testcases to reflect the spec as I understood
> it. If those testcases do not reflect the spec as you understand it,
> then please provide the testcases that demonstrate this.
> 
> Only by collaboration and teamwork will DevelopBranch reach the
> release standards you want.
> 
> > Performance might also suffer since it is faster to read from
> > the hash returned by extractParameters then to create an object
> > and call methods.
> 
> An object in Perl _is_ a hash. There is no difference between saying:
>    $hash->{field}
> and saying
>    $object->{field}
> 
> I deliberately elimnated the use a method to access fields in the
> object hash, because of this concern.
> 
> Note that Rcs*.pm and Meta.pm have been happily using this
> methodology for several years, in far more performance-critical
> domains.
> 
> Regards,
> 
> C.
> 
> --
> C-dot Groupware Consultants
> http://www.c-dot.co.uk

--
   * Peter Thoeny                           Peter@Thoeny.com
   * Is your team already TWiki enabled?    http://TWiki.org
   * This e-mail is:  (_) public  (x) ask first  (_) private