-
-
Notifications
You must be signed in to change notification settings - Fork 32.7k
Fixed #36477, Refs #36163 -- Implemented deprecate_posargs decorator. #19145
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
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
3ee688c
to
0dcb4ae
Compare
a0b3e31
to
cf12809
Compare
An alternative sketch of an idea. And it doesn't do more work than this decorator. (EDIT: actually your decorator does short circuit when overflow positional arguments aren't provided) diff --git a/django/core/mail/__init__.py b/django/core/mail/__init__.py
index b179736b15..8d70282d4b 100644
--- a/django/core/mail/__init__.py
+++ b/django/core/mail/__init__.py
@@ -21,6 +21,7 @@ from django.core.mail.message import (
make_msgid,
)
from django.core.mail.utils import DNS_NAME, CachedDnsName
+from django.utils.deprecation import RemovedInNextVersionWarning
from django.utils.module_loading import import_string
__all__ = [
@@ -139,10 +140,47 @@ def mail_admins(
mail.send(fail_silently=fail_silently)
+import warnings
+
+
+def deprecated_positional_to_kwarg(
+ param_name, args, positional_arg_index, kwarg_value, kwarg_default
+):
+ """
+ Helper function to manage deprecating a positional argument to a keyword argument.
+ """
+ # If we didn't receive enough positional arguments, we can just use the keyword
+ # argument
+ if len(args) < positional_arg_index:
+ return kwarg_value
+
+ # we have at least received a positional argument
+ warnings.warn(
+ f"Passing '{param_name}' as a positional argument is deprecated",
+ RemovedInNextVersionWarning,
+ )
+
+ if kwarg_value is not kwarg_default:
+ raise TypeError(f"got multiple values for argument '{param_name}'")
+ # while it's possible that the value _did_ get passed in by keyword and positionally,
+ # we will close our eyes to this possibility (Model.save deprecation closed its eyes IIRC)
+ return args[positional_arg_index]
+
+
def mail_managers(
- subject, message, fail_silently=False, connection=None, html_message=None
+ subject, message, *args, fail_silently=False, connection=None, html_message=None
):
"""Send a message to the managers, as defined by the MANAGERS setting."""
+ fail_silently = deprecated_positional_to_kwarg(
+ "fail_silently", args, 0, fail_silently, kwarg_default=False
+ )
+ connection = deprecated_positional_to_kwarg(
+ "connection", args, 1, connection, kwarg_default=None
+ )
+ html_message = deprecated_positional_to_kwarg(
+ "html_message", args, 2, html_message,, kwarg_default=None
+ )
+
if not settings.MANAGERS:
return
if not all(isinstance(a, (list, tuple)) and len(a) == 2 for a in settings.MANAGERS):
I think it's possible to make this even cleaner if we used a Here we're looking at 3 lines of code per arg at deprecation, but the code is straightforward, is a common sort of coalescing pattern, and is ultimately temporary code anyways. I did not include the function name in the |
@rtpg interesting approach. I really like that your deprecated_positional_to_kwarg() helper has a much simpler implementation than the decorator. (And wouldn't require a huge test case.) A few minor observations:
Some potentially larger issues: In the forum discussion, there were a number of concerns about going through deprecation at all. See (e.g.,) this from adamchainz. With the decorator, I tried to address each of those. We'd want to consider them for the helper function approach:
|
Note that I have created https://code.djangoproject.com/ticket/36477 as a ticket around having a helper for deprecating positional args in favor of keyword-only args. This is something Natalia and I talked about in the past that "it would be good to have an agreed pattern here", so I'm glad this is being worked on 👍 |
cf12809
to
acfb30b
Compare
I expanded the inline documentation and added a reference to the decorator in the deprecation section in the contributing docs, as suggested in the ticket. I believe this is ready for review. There may be feedback on the content of the deprecation warnings. My goals were:
… but I suspect there's room for improvement. Also, I specifically tried to optimize un-deprecated usage. As much as possible, introspection and message construction is performed either by pre-computing at decorator application time or after detecting the use of deprecated arguments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I think this is really thought through and well tested
I have added mostly stylistic opinions around things like naming and messaging
@nessita I think it would be great for you to also take a look and add any opinions 👍
docs/internals/contributing/writing-code/submitting-patches.txt
Outdated
Show resolved
Hide resolved
I will review this PR as soon as I finish with CSP (which is on the final lap!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @medmunds, this looks amazing 🌟
I suggested a few cosmetic changes and some minimal rewordings. Then perhaps we could add the (what it seems) missing test and adjust the deprecation "location". Let me know what you think!
bf779a9
to
d449066
Compare
Thanks @nessita, that was very helpful! I believe I've addressed everything (except the optional walrus assignment). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @medmunds, this looks fantastic 🌟
I'm pushing some cosmetic edits, including the re-wrapping of comments and ;long strings following the existing style in the Django source code. I'm also nesting the with
calls when there is more than one, and I have added a helper to assert the deprecation warning.
d449066
to
d89ccfe
Compare
Added a `@deprecate_posargs` helper decorator to simplify converting positional-or-keyword parameters to keyword-only, with a deprecation period.
d89ccfe
to
1a51c60
Compare
A
@deprecate_posargs
decorator to simplify converting positional-or-keyword parameters to keyword-only, with a deprecation period.This is related to the discussion around requiring keyword args in much of the django.core.mail API. Companion PR #19186 implements that change, showing use of the decorator (see commit 3665c680).
The consensus in the forum is that these changes can be made without deprecation, so this decorator will not be necessary. This draft PR is just to collect some performance info and facilitate discussion, and I expect to close it without merging.[Edit: consensus has shifted.]Trac ticket number
ticket-36477
Branch description
Implemented a
@deprecate_posargs
decorator to simplify converting positional-or-keyword parameters to keyword-only, with a deprecation period.Checklist
main
branch.