code cleanup by removing redundancy

django-cms / djangocms-versioning

General purpose versioning package for Django CMS 4 and above.

Other

33 stars 29 forks source link

code cleanup by removing redundancy #395

Closed jrief closed 3 months ago

jrief commented 3 months ago

Description

should be rewritten as extension.copy(new_content) because language is redundant in this context, aka. PageContent already knows its language.

[x] I have opened this pull request against master
[ ] I have added or modified the tests when changing logic
[x] I have followed the conventional commits guidelines to add meaningful information into the changelog
[x] I have read the contribution guidelines and I have joined #workgroup-pr-review on Slack to find a “pr review buddy” who is going to review my pull request.

fsbraun commented 3 months ago

@jrief Tests seem to fail.

jrief commented 3 months ago

@jrief Tests seem to fail.

unfortunately I haven't found the PageContent-extension object yet, which overrides method copy() and accepting language. Do I have to patch django-cms as well to fix those tests?

fsbraun commented 3 months ago

Looking at the BaseExtension the signature of its copy method has included the language parameter since 2015. This way page and page content extensions - both subclasses of BaseExtension - can be handled similarly, even though the page content knows its language and the parameter is not needed. There might be third-party apps relying on this, so I am hesitant to patch the core.

jrief commented 3 months ago

Then let's add a deprecation note and leave it as it is.