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

fix: use functools.cached_property to fix propert type annotations for context attributes #2636

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

probablyjassin
Copy link

@probablyjassin probablyjassin commented Nov 2, 2024

fixes #2635 : use functools.cached_property to fix propert type annotations for context attributes

Summary

context attributes like guild, message, user and such have wrong type annotations. They are typed as methods instead of properties.
This is because a NewType based on property was used instead of functools.cached_property, like in other spots in the codebase already.
This PR implements functools.cached_property for context, which fixes these type annotations.

Information

  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed).
  • This PR is not a code change (e.g. documentation, README, typehinting,
    examples, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why.
  • I have updated the changelog to include these changes.

Lulalaby
Lulalaby previously approved these changes Nov 2, 2024
@Lulalaby Lulalaby requested review from plun1331 and JustaSqu1d and removed request for Middledot November 2, 2024 10:33
@Lulalaby Lulalaby enabled auto-merge (squash) November 2, 2024 10:33
CHANGELOG.md Outdated Show resolved Hide resolved
discord/utils.py Show resolved Hide resolved
Co-authored-by: Paillat <[email protected]>
Signed-off-by: Jässin Aouani <[email protected]>
auto-merge was automatically disabled November 2, 2024 11:18

Head branch was pushed to by a user without write access

Copy link
Author

@probablyjassin probablyjassin left a comment

Choose a reason for hiding this comment

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

Ah yeah that makes sense since it's not used anywhere else in the File, I forgot about that.

@Paillat-dev
Copy link
Contributor

@probablyjassin Could you test your code by doing the following?

-class _cached_property:
-    def __init__(self, function):
-        self.function = function
-        self.__doc__ = getattr(function, "__doc__")
-
-    def __get__(self, instance, owner):
-        if instance is None:
-            return self
-
-        value = self.function(instance)
-       setattr(instance, self.function.__name__, value)
-
-        return value

+cached_property = functools.cached_property

if TYPE_CHECKING:
    from typing_extensions import ParamSpec

    from .abc import Snowflake
    from .commands.context import AutocompleteContext
    from .commands.options import OptionChoice
    from .invite import Invite
    from .permissions import Permissions
    from .template import Template

    class _RequestLike(Protocol):
        headers: Mapping[str, Any]

-    cached_property = NewType("cached_property", property)

    P = ParamSpec("P")

else:
-    cached_property = _cached_property
    AutocompleteContext = Any
    OptionChoice = Any

Try running a bot and doing a couple of things. If it works, push it to this pr, I will test it. It would allow to remove entirely pycord's cached_property implementation for the objectively more robust functools one.

@probablyjassin
Copy link
Author

that would be very nice! i'll test it right now!

@probablyjassin
Copy link
Author

probablyjassin commented Nov 8, 2024

@Paillat-dev here is what I found:

Your idea almost works perfectly, except that with these changes, the following line in context.py:

author: Member | User = user

causes this error:

TypeError: Cannot assign the same cached_property to two different names ('user' and 'author').

Full Stack Trace Traceback (most recent call last): File "C:\Users\Jassin\AppData\Local\Programs\Python\Python311\Lib\functools.py", line 976, in __set_name__ raise TypeError( TypeError: Cannot assign the same cached_property to two different names ('user' and 'author').

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
File "C:\Users\Jassin\myprojectfolder\mybotfolder\main.py", line 3, in
import discord
File "C:\Users\Jassin\myprojectfolder\mybotfolder\venv\Lib\site-packages\discord_init_.py", line 34, in

Meaning, if a way can be found to assign ctx.author without just directly assigning user, this problem would be solved entirely.

So in addition to your idea, I tried this and it actually worked out I believe:

# context.py

- author: Member | User = user

+ @cached_property
+    def author(self) -> Member | User:
+        """Returns the user that sent this context's command.
+        Shorthand for :attr:`.Interaction.user`.
+        """
+        return self.interaction.user  # type: ignore # command user will never be None

I tested it on my end and it works well. So if you think this is a good solution, I'll add it to this PR.

@Paillat-dev
Copy link
Contributor

I really start to wonder what the point is of cached_property in here. It has no utility whatsoever and could be replaced with property directly. I will see if I can find places where it serves an actual purpose, if not you might actually be able to remove it entirely everywhere in the codebase and replace it with simple propertyes

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

Successfully merging this pull request may close these issues.

ApplicationContext attributes have wrong type annotations
3 participants