[Zope3-dev] Handling of empty prefixes in zope.formlib and zope.app.form

[Jacob Holm]

Bjorn Tillenius skrev:
> Since no one else commented, I guess I could do it. Personally, I think
> it's OK. As you pointed out, having an id with a leading period is not
> valid XHTML, so I'd be surprised if anyone would depend on it. It'd be
> good to have someone responsible for zope.formlib +1 the patch, but if
> no one gives it -1 one in the near future, it should be OK to merge it
> anyway.
>   
Thank you for taking the time to look at this.
> It's a good sign that no existing tests failed, but you should of
> course add tests for this functionality before merging.
>   
Of course.  I have added the necessary tests in my local checkout. I 
will submit the updated patch as soon as zope.org starts responding again...
>> The patch is minimal in the sense that no API is changed, 
>> only the behavior related to empty prefix strings. Specifically it does 
>> not change the constructor of the (internal?) class 
>> zope.formlib.form.Widgets to take the actual prefix instead of just its 
>> length. Doing this would simplify the code and allow some sanity checks, 
>> but could cause breakage if the class is used anywhere else.
>>     
>
> I think it's good to do it the way you did it. I have an example where
> I use the Widgets class directly, so changing its API would be harder,
> since you'd need to ensure backwards-compatibility.
>   
Harder, but not too hard. Changing the signature to:

    def __init__(self, widgets, prefix_length=None, prefix=None):
        ...

And possibly deprecating the 'prefix_length' argument should do the 
trick.  Since the 'Widgets' class is undocumented, I wouldn't expect 
many uses outside zope.formlib.form, but i may be wrong.  (Note: The 
*class* is undocumented. The interface IWidgets is part of the public 
interface of zope.formlib, but this is irrelevant in in this discussion)
>>  
>> +def prefixjoin(*args):
>> +    return '.'.join(filter(None,args))
>>     
>
> I rarely see filter being used these days. Personally I prefer list
> comprehensions instead.
>
>     return '.'.join([argument for argument in args if argument])
>
> If you use filter there should at least be a space after the comma.
>
>     return '.'.join(filter(None, args))
>   
I personally like 'filter' for being short and efficient, but don't care 
much either way. I agree that the space is needed for readability.
> Although personally I wouldn't have bothered creating 'prefixjoin' at
> all. The code it replaced was so simple, so I wouldn't say it's more
> readable to have it factored it out into a function, and it would have
> made the patch even smaller :) Also, the name of the function isn't
> that clear. Sometimes it's used to join two prefixes, sometimes it's
> used to join a prefix and a field name.
>   
I agree that the name is less than perfect, and would definitely welcome 
suggestions for a better name.
As to whether the function is needed at all...  Having tried both with 
and without, I definitely think that having a separate function helps 
readability.
>> +
>> +def prefixlen(prefix):
>> +    if not prefix:
>> +        return 0
>> +    return len(prefix)+1
>> +
>>  def setUpWidgets(form_fields,
>>     
>
> Normally I would comment on the PEP-8 correctness, and say that the
> top-level functions should be separated by two blank lines. I see that
> you are consistent with the rest of the file, though, so I won't comment
> on it.
Thank you for not commenting then ;-)   I tried to be consistent with 
the existing style.
>   It wouldn't hurt being a bit inconsistent and add docstrings to
> the added functions, though. Understanding the implementation of
> zope.formlib can be quite hard at the moment, since there are
> practically no docstrings at all. This is an excellent example where a
> docstring is needed, since it's not easy to know whether the period
> should be considered part of the prefix or not.
>   
If the Widgets constructor is changed as suggested above, the 
'prefixlen' function could be removed.

That does not change the fact that the code is underdocumented though. I 
have added a docstring to prefixjoin in the patch I am about to upload.

-- 
Med venlig hilsen / Kind regards

Jacob Holm
CTO

Improva ApS
Symbion Science Park
Fruebjergvej 3, boks 46
2100 København Ø

Phone (+45) 3917 9801