Skip to content

Validate imported field values#39

Draft
petschki wants to merge 1 commit into
masterfrom
petschki/issue_38
Draft

Validate imported field values#39
petschki wants to merge 1 commit into
masterfrom
petschki/issue_38

Conversation

@petschki

Copy link
Copy Markdown
Member

fixes #38

@mister-roboto

Copy link
Copy Markdown

@petschki 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:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@petschki

Copy link
Copy Markdown
Member Author

@jenkins-plone-org please run jobs

@petschki petschki requested a review from jensens April 30, 2019 12:28
@petschki

petschki commented Apr 30, 2019

Copy link
Copy Markdown
Member Author

small change, big outcome 🙈 there are a lot validation errors from different packages when validating imported registry.xml values. Just to name one: plone.app.querystring.interfaces.IQueryOperation description field is required but not always filled ... though validation would be nice to have it's not easy to implement.

@petschki petschki requested a review from mauritsvanrees April 30, 2019 12:45
@mauritsvanrees mauritsvanrees marked this pull request as ready for review May 3, 2019 15:45

@mauritsvanrees mauritsvanrees left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Apparently there is a new option to mark PRs as draft, which you did, and I just marked this pull request as ready for review, which was probably the wrong button to push. :-)

I haven't tried it locally, but the idea seems good.

try:
importRegistry(context)
except zope.schema._bootstrapinterfaces.RequiredMissing:
# this should raise RequiredMissing

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can be done with:

with self.assertRaises(zope.schema._bootstrapinterfaces.RequiredMissing):
    importRegistry(context)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thank's for the hint ... I'll change that

@petschki

petschki commented May 4, 2019

Copy link
Copy Markdown
Member Author

@mauritsvanrees draft PRs are quite cool, but once marked as "ready" you cannot go back. This PR is definitely not ready for merging, because it breaks a lot. If we agree to introduce validation of registry values we have to change/fix several other packages to pass all tests again. I think I take it on my Buschenschank todo list ...

@jensens jensens marked this pull request as draft August 26, 2020 13:31
@jensens

jensens commented Dec 26, 2021

Copy link
Copy Markdown
Member

Is this still a a thing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

validate imported values for registry fields

4 participants