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

[IMP] util/misc: cache unique functions #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

diagnoza
Copy link
Contributor

Improves caching to not simply cache the result of the first call of a function, but each unique (based on args and kwargs, taking into consideration their order) call.

Improves caching to not simply cache the result of the first
call of a function, but each *unique* (based on args and kwargs,
taking into consideration their order) call.
@robodoo
Copy link
Contributor

robodoo commented Feb 22, 2024

@diagnoza diagnoza requested review from KangOl, aj-fuentes, cawo-odoo and a team February 22, 2024 21:01
@aj-fuentes
Copy link
Contributor

We are missing here a detailed motivation for this change. I bet you have one :)

@diagnoza
Copy link
Contributor Author

diagnoza commented Feb 22, 2024

We are missing here a detailed motivation for this change. I bet you have one :)

I thought it's so generic and objectively nice that it doesn't need a broader context :p After all, we always want to re-call the method again if any arguments change, right?

I stumbled upon this shortcoming when I modified the dbuuid() method:

def dbuuid(cr):

by adding a new keyword argument. The method is called multiple times during an upgrade and in my use case, at some point, it would've been called with my newly added argument (its value different from the default one). Prior to this PR, the result would be still taken from the first call, completely discarding the fact that this time an argument was changed.

I guess an argument could be made that in the above scenario, such method shouldn't be cached at all. But with this patch we can have the best of both worlds :p

@aj-fuentes
Copy link
Contributor

I thought it's so generic and objectively nice that it doesn't need a broader context :p After all, we always want to re-call the method again if any arguments change, right?

We should have some real issue to address. Please note that current usage of _cached is limited to functions without arguments or taking the generic cr as only argument. Which means that this more general implementation was not necessary till now.

I stumbled upon this shortcoming when I modified the dbuuid() method:

You should include that part in this PR also as a separate commit. The full PR then would be the actual issue plus the generic improvement.

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.

3 participants