z3c.formwidget.query - incorrect interface implementation?
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
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
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
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.
participants (3)
-
Malthe Borch -
Marius Gedminas -
Martin Aspeli