Skip to content

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

Merged
merged 1 commit into from
Jul 16, 2025

Conversation

medmunds
Copy link
Contributor

@medmunds medmunds commented Feb 6, 2025

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

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • n/a I have attached screenshots in both light and dark modes for any UI changes.

@medmunds

This comment was marked as resolved.

@medmunds medmunds force-pushed the deprecate-posargs-decorator branch from 3ee688c to 0dcb4ae Compare February 8, 2025 19:54
@medmunds medmunds marked this pull request as ready for review February 8, 2025 19:56
@medmunds medmunds force-pushed the deprecate-posargs-decorator branch 3 times, most recently from a0b3e31 to cf12809 Compare February 10, 2025 22:18
@rtpg
Copy link
Contributor

rtpg commented Feb 14, 2025

An alternative sketch of an idea.
It makes the deprecation site itself verbose, but (in my opinion) not overly so.

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 NOT_PROVIDED sentinel for the kwargs for a release, but that is not much.

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 TypeError in the same way that the Model.save deprecation code did. I think that not reproducing exactly the same error string for TypeError is good.

@medmunds
Copy link
Contributor Author

@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:

  • If it's meant to be a shared utility in django.core.deprecation, the helper would need to take a warning category param, which adds a little verbosity everywhere it's called. (Or if it's a one-off django.core.mail internal just for this ticket, it could hard-code RemovedInDjango70Warning. It can't use RemovedInNextVersionWarning, which changes over time.)
  • Every function using the helper would also need to check that len(args) is within the number of deprecated positional args, to avoid ticket-35554.
  • The helper issues a separate warning for each deprecated arg used, compared to the decorator's single warning. (I'm not actually sure which is better—I was expecting some iteration on the decorator's messaging during review.)

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:

  • "Fake" signature interferes with IDE help: Adding *args for the helper will prevent IDEs from highlighting deprecated argument usage. (Though that's nowhere near as bad as the completely generic (*args, **kwargs) from the Model.save() deprecation, which I suspect was Adam's concern.) The decorator avoids this by using the real (post-deprecation) signature immediately.

  • Performance: While I really don't believe this is material for the mail functions, there was a concern about deprecation adding overhead. And in most cases the helper approach will add more overhead than the decorator. The decorator adds one function call and an args/kwargs unpacking/repacking per call, in non-deprecated usage. The helper function always adds one function call per deprecated arg whether or not it is used (but no args/kwargs shuffling).

  • Readability/bugs: using the helper function is quite a bit more verbose—and potentially error prone—than the decorator. As you point out, this only lasts 2-3 releases. But it's essentially trading the decorator's complicated implementation (but simple usage) for more complexity in each function with deprecated positional args.

    send_mail() is maybe a worst-case example here. Once blackened, the deprecated args handling adds about 36 lines of code above the original 12-line implementation:

    send_mail() using deprecated_positional_to_kwarg() helper
    def send_mail(
        subject,
        message,
        from_email,
        recipient_list,
        *args,  # RemovedInDjango70Warning: Change to just `*`.
        fail_silently=False,
        auth_user=None,
        auth_password=None,
        connection=None,
        html_message=None,
    ):
        """
        Easy wrapper for sending a single message to a recipient list. All members
        of the recipient list will see the other recipients in the 'To' field.
    
        If from_email is None, use the DEFAULT_FROM_EMAIL setting.
        If auth_user is None, use the EMAIL_HOST_USER setting.
        If auth_password is None, use the EMAIL_HOST_PASSWORD setting.
    
        Note: The API for this method is frozen. New code wanting to extend the
        functionality should use the EmailMessage class directly.
        """
    
        # RemovedInDjango70Warning: Deprecated positional args handling.
        if len(args) > 5:
            raise TypeError(
                "send_mail() takes at most 9 positional arguments"
                " (including 5 deprecated), but %d were given" % (len(args) + 4)
            )
        fail_silently = deprecated_positional_to_kwarg(
            RemovedInDjango70Warning,
            "fail_silently",
            args,
            0,
            fail_silently,
            kwarg_default=False,
        )
        auth_user = deprecated_positional_to_kwarg(
            RemovedInDjango70Warning, "auth_user", args, 1, auth_user, kwarg_default=None
        )
        auth_password = deprecated_positional_to_kwarg(
            RemovedInDjango70Warning,
            "auth_password",
            args,
            2,
            auth_password,
            kwarg_default=None,
        )
        connection = deprecated_positional_to_kwarg(
            RemovedInDjango70Warning, "connection", args, 3, connection, kwarg_default=None
        )
        html_message = deprecated_positional_to_kwarg(
            RemovedInDjango70Warning,
            "html_message",
            args,
            4,
            html_message,
            kwarg_default=None,
        )
        # End of RemovedInDjango70Warning: deprecated positional args handling.
    
        connection = connection or get_connection(
            username=auth_user,
            password=auth_password,
            fail_silently=fail_silently,
        )
        mail = EmailMultiAlternatives(
            subject, message, from_email, recipient_list, connection=connection
        )
        if html_message:
            mail.attach_alternative(html_message, "text/html")
    
        return mail.send()

@sarahboyce
Copy link
Contributor

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 👍

@nessita nessita changed the title Refs #36163 -- Implemented deprecate_posargs decorator. Fixed #36477, Refs #36163 -- Implemented deprecate_posargs decorator. Jun 24, 2025
@medmunds medmunds force-pushed the deprecate-posargs-decorator branch from cf12809 to acfb30b Compare June 24, 2025 20:02
@medmunds
Copy link
Contributor Author

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:

  • Provide actionable "Change to ..." instructions that focus on the specific argument(s) the user needs to update.
  • Mimic the wording of Python's own wrong-number-of-args error messages. (Though Python also switches between "argument" and "arguments"; I just went with "argument(s)" to keep it simpler.)

… 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.

Copy link
Contributor

@sarahboyce sarahboyce 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! 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 👍

@nessita
Copy link
Contributor

nessita commented Jun 25, 2025

I will review this PR as soon as I finish with CSP (which is on the final lap!)

@nessita nessita self-requested a review June 25, 2025 18:48
Copy link
Contributor

@nessita nessita 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 @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!

@medmunds medmunds force-pushed the deprecate-posargs-decorator branch from bf779a9 to d449066 Compare July 9, 2025 01:27
@medmunds
Copy link
Contributor Author

medmunds commented Jul 9, 2025

Thanks @nessita, that was very helpful! I believe I've addressed everything (except the optional walrus assignment).

Copy link
Contributor

@nessita nessita 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 @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.

@nessita nessita force-pushed the deprecate-posargs-decorator branch from d449066 to d89ccfe Compare July 15, 2025 17:19
Added a `@deprecate_posargs` helper decorator to simplify converting
positional-or-keyword parameters to keyword-only, with a deprecation
period.
@nessita nessita force-pushed the deprecate-posargs-decorator branch from d89ccfe to 1a51c60 Compare July 15, 2025 17:21
@nessita nessita merged commit f42b89f into django:main Jul 16, 2025
33 checks passed
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