plone / plone.api

The Plone API
https://6.docs.plone.org/plone.api
Other
88 stars 53 forks source link

Add module to handle relations #449

Closed pbauer closed 3 years ago

pbauer commented 3 years ago

Integrate code from https://github.com/collective/collective.relationhelpers into plone.api Fixes https://github.com/plone/Products.CMFPlone/issues/3137

mister-roboto commented 3 years ago

@pbauer 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!

mauritsvanrees commented 3 years ago

@jensens said:

This one need coordination with plone/Products.CMFPlone#3232. I would propose to get the other PR done, including factoring out utility low level stuff in an own module and avoid repeating complex code doing the same in two places.

So you say: coordinate this with the control panel, use code from there. More specifically: use its relationhelper.py file. Then we get into the question: for which Plone version is this? For plone.api it sounds logical to me to include it in 5.2, so still supporting Python 2.7. But I see the relationhelper.py file already contains Python 3 code, at least f-strings.

I see @Petchesi-Iulian added some except ImportError code to still even support Plone 4.3, probably because tox and Travis still try to run it. That is not needed, I would say. Plone 4.3-5.1 is not supported anymore.

I would prefer to say: make this feature for Plone 5.2 and 6.0. collective.relationhelpers supports 5.2 with Python 2.7, so if both this PR and the control panel PR copy code from there, it should work. The control panel code is probably isolated enough that as release manager I could be persuaded to include it in 5.2, though I am not sure. Or maybe only the relationhelpers.py file (plus tests). But as said, that will already give problems because this file is Python 3 only...

Or do we say: this is only for Plone 6 and on 5.2 you can use collective.relationhelpers?

mauritsvanrees commented 3 years ago

I have merged master in here, so tox and Travis are happier. tox -e lint rightly complains about a TODO item, and this is indeed not finished yet.

pbauer commented 3 years ago

The relation-controlpanel (https://github.com/plone/Products.CMFPlone/pull/3232) is now merged into Plone 6. The relationhelper.py in CMFPlone has no code that is used or needed for this PR in plone.api.

I would prefer to make a version 2.0 of plone.api for Plone 6+ and py3 only. That would also allow us to remove all py2 and AT-compatability code from plone.api. And advise developers to use collective.relationhelpers in older versions.

mauritsvanrees commented 3 years ago

I would prefer to make a version 2.0 of plone.api for Plone 6+ and py3 only. That would also allow us to remove all py2 and AT-compatability code from plone.api. And advise developers to use collective.relationhelpers in older versions.

That makes sense to me.

Sorry, I have not worked on this since March. It is very useful, but it is not very high on my priority list.

pbauer commented 3 years ago

Regarding relations.delete():

pbauer commented 3 years ago

I finished delete as I proposed above and added some more tests. This is now ready to be reviewed.

This branch is on top of https://github.com/plone/plone.api/pull/461 in case we will go ahead with creating a 2.0 release without AT and py2 support. If we decide not to do that I will re-add support for py2 and Plone 5.2 (but obviously not support for AT).

pbauer commented 3 years ago

@jenkins-plone-org please run jobs

ale-rt commented 3 years ago

I rebased this branch on master to have a clean diff and fix some minor details.

It looks good to me, even if there are some small details that should be fixed.

[ale@emily plone.api]$ flake8 src/|grep relation
src/plone/api/tests/test_relation.py:189:5: E303 too many blank lines (2)
src/plone/api/tests/test_relation.py:345:89: E501 line too long (90 > 88 characters)
src/plone/api/tests/test_relation.py:346:89: E501 line too long (89 > 88 characters)
src/plone/api/tests/test_relation.py:364:89: E501 line too long (93 > 88 characters)
src/plone/api/tests/test_relation.py:423:89: E501 line too long (90 > 88 characters)

Not a big deal, we maybe can handle this later by running black on the whole code base.

I would recommend to release the package initially as an alpha to allow the new methods signature to be changed without the need to bump the version number.

jensens commented 3 years ago

@jenkins-plone-org please run jobs

pbauer commented 3 years ago

Thanks for merging 🎉

jensens commented 3 years ago

Thanks for the work!