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

Add deprecated decorators for multiple classes and methods #2458

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

JaeHyuckSa
Copy link
Contributor

I have made things!

This PR adds deprecated decorators to address Django 6.0 deprecations, providing guidance for transitioning to updated methods as noted comment.

Related PR

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thank you!

@sobolevn sobolevn merged commit 438ccfa into typeddjango:master Dec 6, 2024
40 checks passed
@@ -129,6 +129,7 @@ class BaseModelAdmin(Generic[_ModelT]):
def get_prepopulated_fields(self, request: HttpRequest, obj: _ModelT | None = ...) -> dict[str, Sequence[str]]: ...
def get_queryset(self, request: HttpRequest) -> QuerySet[_ModelT]: ...
def get_sortable_by(self, request: HttpRequest) -> _DisplayT[_ModelT]: ...
@deprecated("The None value for the request parameter will be removed in Django 6.0.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be removed. @deprecated can only be used if the whole function is deprecated, not changes to types for individual parameters. This will cause the deprecation to be shown even if request is not None.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, deprecated overloads can be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, yes!

@@ -21,6 +21,7 @@ else:
class IntEnum(int, ReprEnum): ...
class StrEnum(str, ReprEnum): ...

@deprecated("ChoicesMeta is deprecated in favor of ChoicesType and will be removed in Django 6.0.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that this works either as ChoicesType is aliased to ChoicesMeta so will show the same deprecation warning.

As an aside, we should probably also invert the alias so that we have class ChoicesType(EnumType) and ChoicesMeta = ChoicesType for the alias.

@@ -19,6 +20,7 @@ def json_script(value: Any, element_id: str | None = None, encoder: type[JSONEnc

# conditional_escape could use a protocol to be more precise, see https://github.com/typeddjango/django-stubs/issues/1474
def conditional_escape(text: _StrOrPromise | SafeData) -> SafeString: ...
@deprecated("Calling format_html() without passing args or kwargs is deprecated.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, it's not possible to use @deprecated here without false positives.

from typing_extensions import deprecated

@deprecated(
"The django.utils.itercompat.is_iterable() is deprecated and will be removed in Django 6.0. Use isinstance(..., collections.abc.Iterable) instead."
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a lot of "The module.function() is deprecated..." which doesn't read properly. In most cases we can probably drop the "The":

Suggested change
"The django.utils.itercompat.is_iterable() is deprecated and will be removed in Django 6.0. Use isinstance(..., collections.abc.Iterable) instead."
"django.utils.itercompat.is_iterable() is deprecated and will be removed in Django 6.0. Use isinstance(..., collections.abc.Iterable) instead."

Please review all the other introductions of this.

@sobolevn
Copy link
Member

@ngnpope thanks for the detailed review!
@JaeHyuckSa can you please address these comments?

@JaeHyuckSa
Copy link
Contributor Author

@ngnpope thanks for the detailed review! @JaeHyuckSa can you please address these comments?

Thank you so much, @ngnpope, for your detailed and thoughtful review! I really appreciate the time and effort you put into it. I’m currently busy with work, but I’ll make sure to address your comments at the earliest possible opportunity :)

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

Successfully merging this pull request may close these issues.

4 participants