Dx siteroot #27
Dx siteroot #27
Conversation
…y ?> with a VerifyingAdapterLookup, which doesn't have a dependents attribute. Not sure why tf this is happening.
|
@jaroel thanks for creating this Pull Request and help improve Plone! To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass. Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating: With this simple comment all the jobs will be started automatically. Happy hacking! |
|
@jaroel This change could be merged even w/o dx-site-root. It needs a Changelog entry and tests run. |
|
This whole thing can/should be revisited. The latter part of |
There was a problem hiding this comment.
LGTM.
Can probably be merged separately, but we would need to create a branch of supermodel first for Plone 5.2.
But I wonder if this might be save to use on 5.2 as well. The news snippet says it is a breaking change, but to me it only seems a slight performance gain. The original code yields all schemas upfront, but then ignores them anyway when they do not have an attribute _SchemaClass_finalize, and it warns when it is an instance of InterfaceClass instead of SchemaClass.
This could be influenced by the zope.interface version though.
For stability, we should probably not add this in 5.2 though. But if someone wants this, it could be considered.
Okay, I have created branch 1.x and let coredev 5.2 use this.
We could revisit this.
Anyway: it is now probably mergeable to master for Plone 6. Needs another Jenkins run though.
| # to find out why this is happening in the first place. | ||
| try: | ||
| children = schema.dependents.keys() | ||
| except AttributeError: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It seems this branch is needed to fix the plip branch build.
After I added this commit:
- plone/buildout.coredev@f5c817d
I had a succesful build: - https://jenkins.plone.org/job/plip-dx-siteroot-3.9/5/
Before I started:
which is not yet over but I expect it to fail
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@mauritsvanrees I see no harm in doing that, but waiting for other people review seems a good idea: CC @plone/framework-team
|
@jaroel thanks for creating this Pull Request and help improve Plone! To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass. Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating: With this simple comment all the jobs will be started automatically. Happy hacking! |
|
@jaroel thanks for creating this Pull Request and help improve Plone! To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass. Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating: With this simple comment all the jobs will be started automatically. Happy hacking! |
|
I closed and reopened the PR, to see if that would get rid of the proposed Jenkins runs on 5.2, but that is not the case. @jenkins-plone-org please run jobs |
|
Plone 5.2 coredev is on branch 1.x, so this is fine to merge. |
No description provided.