Skip to content

feat: Replaced useless cached_property with property and moved to functools.cached_property #2769

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat: Replaced useless cached_property with property and moved to functools.cached_property #2769

wants to merge 4 commits into from

Conversation

RiccardoVaccari
Copy link

@RiccardoVaccari RiccardoVaccari commented Apr 27, 2025 •
edited
Loading

Summary

Fixes #2660
Replaced all cached_property with property or functools.cached_property based on the solution mentioned in the issue.

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.

@RiccardoVaccari RiccardoVaccari requested review from a team as code owners April 27, 2025 07:20
@pullapprove4 pullapprove4 bot requested a review from BobDotCom April 27, 2025 07:20
@DA-344
Copy link
Contributor

DA-344 commented Apr 27, 2025

See #2636 (comment) and #2636 (comment)

@RiccardoVaccari
Copy link
Author

Do you think is better to replace the two cached_property remained with property or evaluating the use of utils.cached_slots_property?

Paillat-dev
Paillat-dev reviewed Apr 27, 2025
View reviewed changes
@Paillat-dev
Copy link
Contributor

Paillat-dev commented Apr 27, 2025 •
edited
Loading

the two cached_property remained

@RiccardoVaccari So there are two remaining cached_propertys ? Would you mind sending a reference to them ? This should not be merged before they are replaced then.

nvm, I missunderstood, you meant it already uses functools.cached_property. Well in that case we should keep that, as the point of the original issue is to use as much built in code and not custom code as possible, also see as DA344 said.

@DA-344
Copy link
Contributor

DA-344 commented Apr 27, 2025

Do you think is better to replace the two cached_property remained with property or evaluating the use of utils.cached_slots_property?

Keeping them as is it’s fine, it saves a function call because the data where cached_property is used is static, ie created_at, it’s bound to the id, and IDs cannot change, so calculating it everything it gets the property attr is not worth it, and it’s also not that necessary to be stored on a class attribute.

Paillat-dev
Paillat-dev suggested changes Apr 27, 2025
View reviewed changes
discord/poll.py Outdated
@@ -358,7 +358,7 @@ def __init__(
self._expiry = None
self._message = None

@utils.cached_property
@property
Copy link
Contributor

Choose a reason for hiding this comment

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

would benefit imo from functools.cached_property but wait for feedback

Copy link
Author

@RiccardoVaccari RiccardoVaccari Apr 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This one doesn't appear to be a computationally intensive code, but let's wait further feedback.

Copy link
Contributor

@Paillat-dev Paillat-dev Apr 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I just assumed utils.parse_time had enough logic that would make sense to cache it but idk

Copy link
Contributor

Choose a reason for hiding this comment

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

It’d be better if it stayed as a cached_property

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but use the functools one

discord/onboarding.py Outdated
@@ -247,7 +247,7 @@ def _update(self, data: OnboardingPayload):
self.enabled: bool = data["enabled"]
self.mode: OnboardingMode = try_enum(OnboardingMode, data.get("mode"))

@cached_property
@property
def default_channels(
Copy link
Contributor

Choose a reason for hiding this comment

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

would benefit imo from functools.cached_property but wait for feedback

Copy link
Author

Choose a reason for hiding this comment

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

I agree on this one

@RiccardoVaccari
Copy link
Author

Do I need to fix this based on suggestions?

@Paillat-dev
Copy link
Contributor

Paillat-dev commented May 20, 2025 via email •
edited
Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DA-344 DA-344 DA-344 left review comments

@Paillat-dev Paillat-dev Paillat-dev approved these changes

@BobDotCom BobDotCom Awaiting requested review from BobDotCom

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
PA: All Contributors pending PA: Maintainers pending
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace cached_property with property or functools_property
3 participants
@RiccardoVaccari @DA-344 @Paillat-dev