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.

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

Dx siteroot #27

Merged
merged 15 commits into from
Aug 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/27.breaking
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make finalizeSchema more robust by not handling behavior schema classes
18 changes: 16 additions & 2 deletions plone/supermodel/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,22 @@ def finalizeSchemas(parent=Schema):
)

def walk(schema):
yield schema
for child in schema.dependents.keys():
# When we have behaviors on the Plone site root we got some shcmeas that
# are not SchemaClasses
if isinstance(schema, SchemaClass):
jensens marked this conversation as resolved.
Show resolved Hide resolved
yield schema

# This try..except is to handle AttributeError:
# 'VerifyingAdapterLookup' object has no attribute 'dependents'.
# afaik this happens in tests only.
# We have issue https://github.com/plone/plone.supermodel/issues/14
# to find out why this is happening in the first place.
try:
children = schema.dependents.keys()
except AttributeError:
Copy link
Member

Choose a reason for hiding this comment

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

I added a breakpoint here, and it did not get triggered on startup and also not when running the (non-robot) tests from CMFPlone. So there is a chance that this is no longer needed.
But it seems fine for me to keep this, so we avoid weird errors in corner cases in tests.

Copy link
Member

@ale-rt ale-rt Aug 25, 2021

Choose a reason for hiding this comment

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

It seems this branch is needed to fix the plip branch build.
After I added this commit:

Before I started:

which is not yet over but I expect it to fail

Copy link
Member

Choose a reason for hiding this comment

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

It was my mistake to remove this checkout from the PLIP.
I probably got confused between the various packages. Sorry!

Since the job of this individual PR is green on 6.0, and I already let 5.2 use a maintenance branch, it would be fine to merge this PR now, without the other PLIP PRs.
@ale-rt Do you want to push the merge button?

Copy link
Member

@ale-rt ale-rt Aug 25, 2021

Choose a reason for hiding this comment

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

@mauritsvanrees I see no harm in doing that, but waiting for other people review seems a good idea: CC @plone/framework-team

children = ()

for child in children:
for s in walk(child):
yield s

Expand Down