Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix IField.required to not be required by default. #105

Merged
merged 8 commits into from
Feb 9, 2021
Merged

Fix IField.required to not be required by default. #105

merged 8 commits into from
Feb 9, 2021

Conversation

agitator
Copy link
Contributor

@agitator agitator commented Jan 27, 2021 •
edited by icemac
Loading

Fixes #104

@agitator agitator marked this pull request as ready for review January 27, 2021 14:19
icemac
icemac approved these changes Jan 29, 2021
View reviewed changes
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

CHANGES.rst Outdated Show resolved Hide resolved
@icemac icemac changed the title fix for https://github.com/zopefoundation/zope.schema/issues/104 Fix IField.required to not be required by default. Jan 29, 2021
jamadden
jamadden approved these changes Jan 29, 2021
View reviewed changes
Copy link
Member

@jamadden jamadden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like potentially a large semantic change (not everything pays attention to defaults), suggesting to me that the next release should be 6.1.0.

Could there be a test added to check not IField['required'].required? Otherwise this could easily be changed back.

@agitator
Copy link
Contributor Author

agitator commented Feb 3, 2021

double checked in Plone content type schema editor - works now as expected

jensens
jensens approved these changes Feb 3, 2021
View reviewed changes
Copy link
Member

@jensens jensens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Anyway, self referencing implementations are hard. In interfaces.py line 410 required=False is redundant, but anyway, for the sake of being explicit (and documentation) I think this is fine.

@pbauer
Copy link
Member

pbauer commented Feb 3, 2021 •
edited
Loading

This breaks the schema in eea.facettednavigation in a way that I do not understand:

2021-02-03 13:34:23,251 INFO    [chameleon.config:38][MainThread] directory cache: /Users/pbauer/workspace/dynajet_relaunch/var/cache.
Traceback (most recent call last):
  File "/Users/pbauer/.cache/buildout/eggs/zope.configuration-4.4.0-py3.8.egg/zope/configuration/config.py", line 1687, in toargs
    args[str(name)] = field.fromUnicode(s)
  File "/Users/pbauer/.cache/buildout/eggs/zope.configuration-4.4.0-py3.8.egg/zope/configuration/fields.py", line 172, in fromUnicode
    value = self.context.resolve(name)
  File "/Users/pbauer/.cache/buildout/eggs/zope.configuration-4.4.0-py3.8.egg/zope/configuration/config.py", line 226, in resolve
    __import__(mname)
  File "/Users/pbauer/workspace/dynajet_relaunch/src-mrd/eea.facetednavigation/eea/facetednavigation/subtypes/interfaces.py", line 25, in <module>
    class IFacetedSubtyper(Interface):
  File "/Users/pbauer/workspace/dynajet_relaunch/src-mrd/eea.facetednavigation/eea/facetednavigation/subtypes/interfaces.py", line 29, in IFacetedSubtyper
    can_enable = schema.Bool(u'Can enable faceted navigation',
  File "/Users/pbauer/workspace/dynajet_relaunch/src-mrd/zope.schema/src/zope/schema/_bootstrapfields.py", line 614, in __init__
    super(Bool, self).__init__(required=required, **kw)
  File "/Users/pbauer/workspace/dynajet_relaunch/src-mrd/zope.schema/src/zope/schema/_bootstrapfields.py", line 269, in __init__
    self.required = required
  File "/Users/pbauer/workspace/dynajet_relaunch/src-mrd/zope.schema/src/zope/schema/fieldproperty.py", line 84, in __set__
    field.validate(value)
  File "/Users/pbauer/workspace/dynajet_relaunch/src-mrd/zope.schema/src/zope/schema/_bootstrapfields.py", line 300, in validate
    self._validate(value)
  File "/Users/pbauer/workspace/dynajet_relaunch/src-mrd/zope.schema/src/zope/schema/_bootstrapfields.py", line 621, in _validate
    Field._validate(self, value)
  File "/Users/pbauer/workspace/dynajet_relaunch/src-mrd/zope.schema/src/zope/schema/_bootstrapfields.py", line 349, in _validate
    raise WrongType(
zope.schema._bootstrapinterfaces.WrongType: ('Can enable faceted navigation', <class 'bool'>, 'required')

The schema in question looks like this:

class IFacetedSubtyper(Interface):
    """ Support for subtyping objects
    """

    can_enable = schema.Bool(u'Can enable faceted navigation',
                             readonly=True)
    can_disable = schema.Bool(u'Can disable faceted navigation',
                              readonly=True)
    is_faceted = schema.Bool(u'Is current object faceted navigable',
                             readonly=True)
    is_lingua_faceted = schema.Bool(
        u'Is LinguaPlone installed and current object is faceted navigable',
        readonly=True)

@pbauer
Copy link
Member

pbauer commented Feb 3, 2021

@agitator: Thanks for fixing my problem.

@icemac
Copy link
Member

icemac commented Feb 9, 2021

Is there still something to be done before this PR can be merged?

@agitator
Copy link
Contributor Author

agitator commented Feb 9, 2021

no other problems found so far, should be fine

@icemac icemac merged commit fa1eb0c into master Feb 9, 2021
@icemac icemac deleted the required-bool-required branch February 9, 2021 07:24
@icemac
Copy link
Member

icemac commented Feb 9, 2021

Just released as https://pypi.org/project/zope.schema/6.1.0/ to PyPI.

icemac referenced this pull request in zopefoundation/zope.formlib Feb 15, 2021
@dataflake
- Adjust checkbox widget test to new default for required on bool…
e55d8ae 
…ean fields
jensens added a commit that referenced this pull request Aug 19, 2021
@jensens
revert parts of #105 due to incompatibility for some use cases.
0324afa
jensens added a commit that referenced this pull request Aug 19, 2021
@jensens
revert parts of #105 due to incompatibility
0dc25a4 
see also issue #104
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jensens jensens jensens approved these changes

@icemac icemac icemac approved these changes

@jamadden jamadden jamadden approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

IField.required attribute is True but must not.
5 participants
@agitator @pbauer @icemac @jensens @jamadden