z3c.formwidget.query - incorrect interface implementation?

[Martin Aspeli]

Hi,

I'm trying to build a widget that allows for auto-complete of items in a
vocabulary, but also allows additional (string) values to be added. To
understand how that works, I am digging into z3c.formwidget.query, and
it looks to me like it's making incorrect assumptions about its own
interfaces.

In its interfaces.py, we have:

from zope.schema.interfaces import ISource, IVocabularyTokenized
class IQuerySource(ISource, IVocabularyTokenized):
     def search(query_string):
         """Return values that match query."""

so, an IQuerySource is an ISource, which is:

class ISource(Interface):

     def __contains__(value):
         """Return whether the value is available in this source
         """

and also an IVocabularyTokenized, which is:

class IBaseVocabulary(ISource):

     def getTerm(value):
         """Return the ITerm object for the term 'value'.

         If 'value' is not a valid term, this method raises LookupError.
         """

# BBB vocabularies are pending deprecation, hopefully in 3.3
class IIterableVocabulary(Interface):

     def __iter__():
         """Return an iterator which provides the terms from the
vocabulary."""

     def __len__():
         """Return the number of valid terms, or sys.maxint."""

class IVocabulary(IIterableVocabulary, IBaseVocabulary):
     """Vocabulary which is iterable."""

class IVocabularyTokenized(IVocabulary):

     def getTermByToken(token):
         """Return an ITokenizedTerm for the passed-in token.

         If `token` is not represented in the vocabulary, `LookupError`
         is raised.
         """

First of all, it seems strange then that IQuerySource should explicitly
need to say it's derived from ISource since IVocabularyTokenized already
promises that.

However, more importantly, in the README.txt of z3c.formwidget.query we
have this:

   ...     def getTermByValue(self, value):
   ...         return self.vocabulary.by_value[value]

and similarly, in widget.py:

         map(terms.add,
             map(source.getTermByValue,
                 filter(lambda value: value and value not in values,
selection)))

If I'm not reading this wrong, it seems to me that z3c.formwidget.query
is using getTermByValue() (which I can't find anywhere else) instead of
getTermByToken() as defined by IVocabularyTokenized.

Is this correct?

Martin

--
Author of `Professional Plone Development`, a book for developers who
want to work with Plone. See http://martinaspeli.net/plone-book

_______________________________________________
Zope-Dev maillist  -  Zope-Dev@...
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists -
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )

[Malthe Borch-2]

Martin Aspeli wrote:
> I'm trying to build a widget that allows for auto-complete of items in a
> vocabulary, but also allows additional (string) values to be added. To
> understand how that works, I am digging into z3c.formwidget.query, and
> it looks to me like it's making incorrect assumptions about its own
> interfaces.

This can very well be; the defense would be that the entire vocabulary
code is full of odd interfaces that don't really form a complete story.

> If I'm not reading this wrong, it seems to me that z3c.formwidget.query
> is using getTermByValue() (which I can't find anywhere else) instead of
> getTermByToken() as defined by IVocabularyTokenized.

I don't remember the details, but the implementation may be incorrect
although functional. Certainly, it's made to good use by
``plone.app.z3cform`` and a number of other modules.

You're more than welcome to improve or devise an alternative implementation.

\malthe

_______________________________________________
Zope-Dev maillist  -  Zope-Dev@...
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists -
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )

[Martin Aspeli]

Malthe Borch wrote:
> Martin Aspeli wrote:
>> I'm trying to build a widget that allows for auto-complete of items in a
>> vocabulary, but also allows additional (string) values to be added. To
>> understand how that works, I am digging into z3c.formwidget.query, and
>> it looks to me like it's making incorrect assumptions about its own
>> interfaces.
>
> This can very well be; the defense would be that the entire vocabulary
> code is full of odd interfaces that don't really form a complete story.

For what it's worth, I agree. :-)

>> If I'm not reading this wrong, it seems to me that z3c.formwidget.query
>> is using getTermByValue() (which I can't find anywhere else) instead of
>> getTermByToken() as defined by IVocabularyTokenized.
>
> I don't remember the details, but the implementation may be incorrect
> although functional. Certainly, it's made to good use by
> ``plone.app.z3cform`` and a number of other modules.
>
> You're more than welcome to improve or devise an alternative implementation.

Have you ever tested it with a source other than the one in
queryselect's example code and the one in plone.app.z3cform.queryselect?
Those are the only places I can see getTermByValue().

The fix would probably be to make sure it uses tokens everywhere and
implement getTermByToken.

Martin

--
Author of `Professional Plone Development`, a book for developers who
want to work with Plone. See http://martinaspeli.net/plone-book

_______________________________________________
Zope-Dev maillist  -  Zope-Dev@...
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists -
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )

[Marius Gedminas-4]

On Sun, Aug 24, 2008 at 07:30:24PM +0100, Martin Aspeli wrote:
> I'm trying to build a widget that allows for auto-complete of items in a
> vocabulary, but also allows additional (string) values to be added. To
> understand how that works, I am digging into z3c.formwidget.query, and
> it looks to me like it's making incorrect assumptions about its own
> interfaces.
<snip>
> class IBaseVocabulary(ISource):
>
>      def getTerm(value):
>          """Return the ITerm object for the term 'value'.
>
>          If 'value' is not a valid term, this method raises LookupError.
>          """
<snip>
> class IVocabularyTokenized(IVocabulary):
>
>      def getTermByToken(token):
>          """Return an ITokenizedTerm for the passed-in token.
>
>          If `token` is not represented in the vocabulary, `LookupError`
>          is raised.
>          """
<snip>

> However, more importantly, in the README.txt of z3c.formwidget.query we
> have this:
>
>    ...     def getTermByValue(self, value):
>    ...         return self.vocabulary.by_value[value]
>
> and similarly, in widget.py:
>
>          map(terms.add,
>              map(source.getTermByValue,
>                  filter(lambda value: value and value not in values,
> selection)))
>
> If I'm not reading this wrong, it seems to me that z3c.formwidget.query
> is using getTermByValue() (which I can't find anywhere else) instead of
> getTermByToken() as defined by IVocabularyTokenized.

That does look like a bug.  However, without looking very deeply at the
code,the fix would be to use getTerm, not getTermByToken.

I wouldn't mind seeing getTerm renamed to getTermByValue, but that's
probably difficult to do without breaking backwards-compatibility.

Marius Gedminas
--
I used to think I was indecisive, but now I'm not so sure.


_______________________________________________
Zope-Dev maillist  -  Zope-Dev@...
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists -
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )