Re: Request for comment on zope.schema patch for bug 969350

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:

> 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 ;)

> +# 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.

> +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
> +
> +
> +@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()))
> +
> +
> +@implementer(IMultipleChoice)
> +class Choices(object):
> +
> +    def __iter__(self):
> +        return iter(range(5))  # to have EnumSource work

EnumSource?  Do you mean EnumContext?

> +
> +    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()?

> +        # 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'?

> +        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.

> +    """This class proxies a schema to get its fields binded to a context

s/binded/bound/

> +    """
> +
> +    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))

?

> +
> +    # 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.

> +
> +
>  @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
-- 
http://pov.lt/ -- Zope 3/BlueBream consulting and development

_______________________________________________
Zope-Dev maillist  -  [email protected]
https://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists -
 https://mail.zope.org/mailman/listinfo/zope-announce
 https://mail.zope.org/mailman/listinfo/zope )

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iEYEARECAAYFAk92HZEACgkQkVdEXeem14/6hACghhGnW0dEWflc1yY7/X8n+xTj
YHoAniuOMcDpBBNbt5oxDV0WhHf2hqV9
=sgMD
-----END PGP SIGNATURE-----