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

more flexible cached_member decorator #187

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Conversation

jonasleitner
Copy link
Contributor

@jonasleitner jonasleitner commented Oct 30, 2024

There are 2 problems with the current version (see example below):

  • It is not possible to call a decorated function with keyword arguments (quality of life)
  • The decorator does not work with default arguments and might unexpectedly not load results from the cache.

Neither of the points is currently a problem. Just to have it sorted for the future.
The new version should either work as intended (including the 2 points listed above) or raise an exception if a member function can't be wrapped with the decorator.

from adcc.misc import cached_member_function

class Test:
    @cached_member_function
    def foo(self, x, y = 1):
        print(x, y)

t = Test()

# this will not work
t.foo(x=2)

# this will print twice because the default argument
# is not correctly considered
t.foo(0)  # key for cache lookup: (0,)
t.foo(0, 1)  # key for cache lookup: (0, 1)

@jonasleitner
Copy link
Contributor Author

@apapapostolou @maxscheurer can one of you take a quick look?
Since I'm using this code snippet in the equation generator I am pretty sure everything should work.

@maxscheurer
Copy link
Member

I'm gonna play the bad guy 😂
Can you please add a unit test that tests the functionality and failures of the edge cases, e.g. kwarg only etc? Just a suggestion...

@jonasleitner
Copy link
Contributor Author

Should have seen that one coming 😂👌
I think this should cover most cases. Did I miss something?

@apapapostolou apapapostolou merged commit e2e1bce into master Nov 5, 2024
8 checks passed
@apapapostolou apapapostolou deleted the cached_member branch November 5, 2024 09:35
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