[Zope-dev] Request for comment on zope.schema patch for bug 969350

[Alexandre Garel]

Le 30/03/2012 22:54, Marius Gedminas a écrit :
> On Fri, Mar 30, 2012 at 06:36:13PM +0200, Alexandre Garel wrote:
>> Hello,
>>
>> I fill a bug report for zope.schema :
>>
>> "zope.schema does not handle well nested object fields"
>> https://bugs.launchpad.net/zope.schema/+bug/969350
>>
>> my patch change a particular behaviour (binding of Choice field
>> nested in Object field)
>>
>> I explain why on the ticket.
>>
>> Any comment would be appreciated.
> Here's the diff attached to that bug:

Thanks for taking time.

As you can see below I'm ok for all your comment but one, which needs 
further discussion.

>> Index: src/zope/schema/tests/test_objectfield_nested.py
>> ===================================================================
>> --- src/zope/schema/tests/test_objectfield_nested.py	(révision 0)
>> +++ src/zope/schema/tests/test_objectfield_nested.py	(révision 0)
>> @@ -0,0 +1,74 @@
>> +##############################################################################
>> +#
>> +# Copyright (c) 2001, 2002 Zope Foundation and Contributors.
> Probably should be 2012 ;)

Hum copy/paste killed me :-)

>> +# All Rights Reserved.
>> +#
>> +# This software is subject to the provisions of the Zope Public License,
>> +# Version 2.1 (ZPL).  A copy of the ZPL should accompany this distribution.
>> +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED
>> +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
>> +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS
>> +# FOR A PARTICULAR PURPOSE.
>> +#
>> +##############################################################################
>> +"""This test exercises Object fields in the case of nestment inside list
> s/netsment/nesting/, although the sentence could use some rewriting.

ok for nesting
>> +to verify binding correcly happens at validation time
>> +"""
>> +
>> +from unittest import TestCase, TestSuite, main, makeSuite
>> +
>> +from zope.interface import Interface, implementer
>> +from zope.schema import Choice, Object, Set
>> +from zope.schema.interfaces import IContextSourceBinder, WrongContainedType
>> +from zope.schema.vocabulary import SimpleVocabulary, SimpleTerm
>> +from zope.testing.cleanup import CleanUp
>> +
>> +
>> + at implementer(IContextSourceBinder)
>> +class EnumContext(object):
>> +
>> +    def __call__(self, context):
>> +        return SimpleVocabulary([
>> +            SimpleTerm(value=v, token=v, title='Letter %s' % v)
>> +            for v in context])
>> +
>> +
>> +class IMultipleChoice(Interface):
>> +    choices = Set(value_type=Choice(source=EnumContext()))
>> +
>> +
>> + at implementer(IMultipleChoice)
>> +class Choices(object):
>> +
>> +    def __iter__(self):
>> +        return iter(range(5))  # to have EnumSource work
> EnumSource?  Do you mean EnumContext?

Yes, I'll correct

>> +
>> +    def __init__(self, choices):
>> +        self.choices = choices
>> +
>> +
>> +class IFavorites(Interface):
>> +    fav = Object(title=u"Favorites number", schema=IMultipleChoice)
>> +
>> +
>> +class ObjectNestedTest(CleanUp, TestCase):
>> +    """Test the Object Field in cases of nested fields"""
>> +
>> +    def test_validate_nested(self):
>> +        context = range(5)
> This is a bit confusing.  IField.bind() is usually passed a context that
> is an object that has an attribute with that field.  You're passing a
> list of numbers.  Would the test also work if you had context = object()?
In fact as I just wan't to test validation, I did use a range. I would 
of course work with an object which is iterable (because EnumContext 
needs it)
>> +        # must not raise
>> +        IFavorites['fav'].bind(context).validate(Choices(set([1, 3])))
>> +        # checking against dictionnary do work
> I don't see any dictionaries...  Perhaps you meant 'checking of set
> values against the source works'?
Hum sorry, that may be a copy/paste remainder there again :-/

>> +        self.assertRaises(
>> +            WrongContainedType,
>> +            IFavorites['fav'].bind(context).validate, Choices(set([1, 8])))
>> +
>> +
>> +def test_suite():
>> +    suite = TestSuite()
>> +    suite.addTest(makeSuite(ObjectNestedTest))
>> +    return suite
>> +
>> +
>> +if __name__ == '__main__':
>> +    main(defaultTest='test_suite')
>> Index: src/zope/schema/_field.py
>> ===================================================================
>> --- src/zope/schema/_field.py	(révision 124803)
>> +++ src/zope/schema/_field.py	(copie de travail)
>> @@ -492,12 +492,7 @@
>>               if not IMethod.providedBy(schema[name]):
>>                   try:
>>                       attribute = schema[name]
>> -                    if IChoice.providedBy(attribute):
>> -                        # Choice must be bound before validation otherwise
>> -                        # IContextSourceBinder is not iterable in validation
>> -                        bound = attribute.bind(value)
>> -                        bound.validate(getattr(value, name))
>> -                    elif IField.providedBy(attribute):
>> +                    if IField.providedBy(attribute):
>>                           # validate attributes that are fields
>>                           attribute.validate(getattr(value, name))
>>                   except ValidationError as error:
>> @@ -510,6 +505,24 @@
>>       return errors
>>
>>
>> +class BindedSchema(object):
> BoundSchema would be a more grammatically correct name.

Ok I'll change that

>> +    """This class proxies a schema to get its fields binded to a context
> s/binded/bound/
ok

>> +    """
>> +
>> +    def __init__(self, schema, context):
>> +        self.schema = schema
>> +        self.context = context
>> +
>> +    # we redefine getitem
>> +    def __getitem__(self, name):
>> +        attr = self.schema[name]
>> +        return attr.bind(self.context)
> Shouldn't this be
>
>             return attr.bind(getattr(self.context, name))

That's what I discuss on the ticket. For me binding have to give a 
context for vocabulary. The problem is that validation has to take place 
at initialization time. The object may have no attribute 'name' at this 
time.

For example in cromlech.forms, or zeam.forms in the case of adding an 
object context is an Adder instance, not an object of the expected type.

Thus I prefer to give the upper context for binding of vocabularies, but 
I maybe wrong.



>> +
>> +    # but let all the rest slip to schema
>> +    def __getattr__(self, name):
>> +        return getattr(self.schema, name)
> The comments don't really add anything to the code.

Ok.

>> +
>> +
>>   @implementer(IObject)
>>   class Object(Field):
>>       __doc__ = IObject.__doc__
>> @@ -521,6 +534,11 @@
>>           self.schema = schema
>>           super(Object, self).__init__(**kw)
>>
>> +    def bind(self, context=None):
>> +        binded = super(Object, self).bind(context)
>> +        binded.schema = BindedSchema(self.schema, context)
> s/binded/bound/
>
>> +        return binded
>> +
>>       def _validate(self, value):
>>           super(Object, self)._validate(value)
> Marius Gedminas
>