upgrade-util icon indicating copy to clipboard operation
upgrade-util copied to clipboard

[IMP] util/misc: cache unique functions

Open diagnoza opened this issue 1 year ago • 4 comments

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.

diagnoza avatar Feb 22 '24 21:02 diagnoza

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

aj-fuentes avatar Feb 22 '24 21:02 aj-fuentes

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: https://github.com/odoo/upgrade-util/blob/aa7131eaae7659fcc62182257ef7e0ea533aacf9/src/util/specific.py#L13

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

diagnoza avatar Feb 22 '24 21:02 diagnoza

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.

aj-fuentes avatar Feb 23 '24 15:02 aj-fuentes