On Thu, Sep 28, 2006 at 11:34:30PM +0200, Jacob Holm wrote:
> Would it be OK to change the handling of empty prefixes in zope.formlib
> and zope.app.form to *not* add that leading period in case of an empty
> prefix? The current behavior is undocumented and could be considered a
> bug (if not in the code then in the documentation and tests).
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.
> I have attached a small patch against the current trunk that does what I
> want. All current tests pass, but no tests for the new behavior has been
> added (yet).
It's a good sign that no existing tests failed, but you should of
course add tests for this functionality before merging.
> 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.
> Comments?
The patch looks non-intrusive, i.e, as you said no API is changed, so
it shouldn't break anyone's code. I have a few minor comments on the
code itself.
> Index: src/zope/formlib/form.py
> ===================================================================
> --- src/zope/formlib/form.py (revision 70426)
> +++ src/zope/formlib/form.py (working copy)
> @@ -208,6 +208,14 @@
> return zope.security.canAccess(context, writer.__name__)
> return zope.security.canWrite(context, field.__name__)
>
> +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))
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.
> +
> +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. 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.
Regards,
Bjorn
_______________________________________________
Zope3-dev mailing list
Zope3-dev@zope.org
Unsub: http://mail.zope.org/mailman/options/zope3-dev/archive%40mail-archive.com
