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

Make the dependency on plone.app.content conditional. #380

Merged
merged 2 commits into from
Nov 2, 2023

Conversation

mauritsvanrees
Copy link
Sponsor Member

This is for INameFromTitle, which we want to move to plone.base. See #379 (comment) for why this is not working yet, and we need this workaround.

This is for `INameFromTitle`, which we want to move to `plone.base`.
See #379 (comment) for why this is not working yet, and we need this workaround.
@mister-roboto

This comment was marked as resolved.

@mauritsvanrees
Copy link
Sponsor Member Author

@jenkins-plone-org please run jobs

Copy link
Sponsor Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

@mauritsvanrees While reading your comments on #379 I had the same idea that this could be a simpler way to resolve the circular dependency -- before I saw this PR. So, I think this makes sense.

Both `plone.namefromtitle` and `plone.app.content.interfaces.INameFromTitle` work.
You cannot use `plone.base.interfaces.INameFromTitle` as behavior name, this was never supported.
Recommended is to use `plone.namefromtitle` in all supported Plone versions.
Using `plone.app.content.interfaces.INameFromTitle` as name will stop working in Plone 7.

Rewrote the `tests/namefromtitle.txt` doctest file as a unittest file, making it easier to test both behavior names.
@mauritsvanrees
Copy link
Sponsor Member Author

I have updated the tests to test for both supported spellings of the behavior.
This works in combination with the updated plone/plone.app.content#271 where I remove the deferred import, and inherit from the plone.base interface instead.
That seems the best we can do.
I will test them together on Jenkins.

@mauritsvanrees mauritsvanrees merged commit 669e1e7 into master Nov 2, 2023
13 checks passed
@mauritsvanrees mauritsvanrees deleted the maurits-plone-app-content-conditional branch November 2, 2023 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants