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

Delete deprecated argument #42

Merged
merged 5 commits into from
Nov 22, 2024
Merged

Delete deprecated argument #42

merged 5 commits into from
Nov 22, 2024

Conversation

jfalfaro
Copy link
Contributor

@jfalfaro jfalfaro commented Nov 1, 2021

Support for context argument was removed in Django 3.0

@NicolaiRidani NicolaiRidani requested a review from a team November 1, 2021 19:10
@vinitkumar
Copy link
Member

@heller166 Thanks for contributing, Can you please write this code in a backward compatible way so that it doesn't break for old version of Django and works for the newer versions too?

@jfalfaro
Copy link
Contributor Author

jfalfaro commented Nov 1, 2021

I'm not sure that I can do that now that. I was trying to avoid getting a RemovedInDjango30Warning that is issued in the following code. The function that is executed to check if context is present checks the method signature and I can't come up with a way for it to be removed while keeping backwards compatibility.

def func_supports_parameter(func, parameter):
    return parameter in inspect.signature(func).parameters

@jfalfaro jfalfaro closed this Nov 1, 2021
@vinitkumar vinitkumar reopened this Nov 2, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2021

Codecov Report

Merging #42 (0076a64) into master (d55e448) will not change coverage.
The diff coverage is n/a.

❗ Current head 0076a64 differs from pull request most recent head 5c257d9. Consider uploading reports for the commit 5c257d9 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master      #42   +/-   ##
=======================================
  Coverage   90.25%   90.25%           
=======================================
  Files           3        3           
  Lines         154      154           
  Branches       26       26           
=======================================
  Hits          139      139           
  Misses         10       10           
  Partials        5        5           
Impacted Files Coverage Δ
djangocms_attributes_field/fields.py 87.17% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d55e448...5c257d9. Read the comment docs.

@vinitkumar
Copy link
Member

@heller166 You did't had to close your pull request.

Check this code, how we can fix some deprecation warnings or errors using this conditionals, and also this:

from platform import python_version
from django import get_version

from distutils.version import LooseVersion


DJANGO_VERSION = get_version()
PYTHON_VERSION = python_version()

# These means "less than or equal to DJANGO_FOO_BAR"
DJANGO_2_2 = LooseVersion(DJANGO_VERSION) < LooseVersion('3.0')
DJANGO_3_0 = LooseVersion(DJANGO_VERSION) < LooseVersion('3.1')
DJANGO_3_1 = LooseVersion(DJANGO_VERSION) < LooseVersion('3.2')
DJANGO_3_2 = LooseVersion('3.2') <= LooseVersion(DJANGO_VERSION) and  LooseVersion(DJANGO_VERSION) < LooseVersion('3.3')

django-cms/django-cms@cc394d9#diff-5452cbe13e8c4195320b722d63a033d4670ecd0d57a3ba6772d1fd1152fd4d58L40

@jfalfaro
Copy link
Contributor Author

jfalfaro commented Nov 2, 2021

@vinitkumar thanks for the suggestion. I implemented a solution, let me know what you think. I tried to come up with some way to test this but it seemed a bit more trouble than its worth.

@@ -10,6 +10,8 @@
from django.utils.translation import ugettext as _
from django.utils.translation import ugettext_lazy

from cms.utils.compat import DJANGO_2_2
Copy link
Member

Choose a reason for hiding this comment

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

This file doesn't exists in the repo, just copy the same logic into a utils file and use that to import and this should be fine.

@marksweb
Copy link
Member

@vinitkumar looks like there has been some work since your comments so could you review again?

@vinitkumar
Copy link
Member

@fsbraun The change looks good here. Should we merge it?

@marksweb
Copy link
Member

@vinitkumar could merge it so the author gets their contribution in and then extend it because the support brought in should be the standard now with nobody using 2.2

@vinitkumar
Copy link
Member

@marksweb Roger that.

@vinitkumar vinitkumar merged commit ebae665 into django-cms:master Nov 22, 2024
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.

4 participants