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
___
Zope3-dev mailing list
Zope3-dev@zope.org
Unsub: http://mail.zope.org/mailman/options/zope3-dev/archive%40mail-archive.com
