Solution#2247
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The request meets the required criteria for approval.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| # Write your code here | ||
| pass | ||
| @wraps(func) | ||
| def wrapper(*args, **kwargs) -> None: |
There was a problem hiding this comment.
The return type annotation -> None is inaccurate. The wrapper should return whatever the decorated function returns (e.g., int, list), not None. Consider using a generic type annotation like @overload or simply removing the annotation.
| pass | ||
| @wraps(func) | ||
| def wrapper(*args, **kwargs) -> None: | ||
| curr_dict = {} |
There was a problem hiding this comment.
This line initializes curr_dict = {} which is immediately overwritten on line 13. This is redundant code that violates the DRY principle. Initialize curr_dict only when needed.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The cache decorator implementation has critical issues that need to be fixed before it can be approved. Primary Issue: kwargs ignored in cache key. The decorator only uses tuple(args) when creating cache keys, completely ignoring keyword arguments. This means calls like func(1, 2) and func(1, 2, extra=3) would incorrectly share the same cached result. The cache key should include both positional and keyword arguments, for example: (*args, tuple(sorted(kwargs.items()))). Secondary Issue: Cache not properly isolated between tests. The test failure shows that the first call to long_time_func(1, 2, 3) returns "Getting from cache" instead of "Calculating new result", indicating stale cache data from previous test runs. The decorator appears to be using a global/shared cache that persists between tests. Each decorated function needs its own isolated cache space, typically keyed by func.name or function identity. Suggested fixes: 1. For the kwargs issue, update your cache key generation to something like: cache_key = (func.name, tuple(args), tuple(sorted(kwargs.items()))). 2. For the test isolation issue, ensure cache entries are properly namespaced by function and cleared between test runs (or use function-scoped storage). Both issues prevent the cache decorator from working correctly and cause the unit test failures. Please address these before resubmitting.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| def wrapper(*args, **kwargs) -> Any: | ||
| if cache_dict.get(func.__name__, None) is None: | ||
| cache_dict[func.__name__] = {} | ||
| curr_dict = cache_dict[func.__name__] | ||
| if curr_dict.get(tuple(args), None) is None: |
There was a problem hiding this comment.
The cache key only uses tuple(args) and ignores **kwargs. If a function is called with func(1, 2) and then func(1, 2, extra=3), the cache key collision will cause incorrect behavior. The cache key should include kwargs as well, for example: (*args, tuple(sorted(kwargs.items())))
| curr_dict = cache_dict[func.__name__] | ||
| if curr_dict.get(tuple(args), None) is None: | ||
| print("Calculating new result") | ||
| curr_dict[tuple(args)] = func(*args, **kwargs) |
There was a problem hiding this comment.
The same issue exists here - only args is used in the cache key, ignoring kwargs entirely
| print("Calculating new result") | ||
| curr_dict[tuple(args)] = func(*args, **kwargs) | ||
| else: | ||
| print("Getting from cache") |
There was a problem hiding this comment.
And here - the return value lookup also ignores kwargs
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution despite the identified bugs, as this is an educational context where learning should be encouraged. You've successfully implemented the basic decorator structure with proper function isolation using func.__name__, which is a solid foundation. However, there are two critical bugs you must fix: 1. Cache key ignores kwargs values: In both cache implementations, tuple(kwargs) only extracts the keys (parameter names), not their values. This means func(a=1) and func(a=2) incorrectly share the same cache entry. Use tuple(sorted(kwargs.items())) instead to include both keys and values. 2. Missing function namespacing in second implementation: If you have two different decorator definitions, ensure both use func.__name__ to separate caches between functions. Your current implementation would pass tests with only positional args but fail when keyword arguments have different values. These fixes will make your cache decorator work correctly across all argument combinations. Keep going—debugging caching logic is a valuable skill!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| if cache_dict.get(func.__name__, None) is None: | ||
| cache_dict[func.__name__] = {} | ||
| curr_dict = cache_dict[func.__name__] | ||
| if curr_dict.get(tuple(args) + tuple(kwargs), None) is None: |
There was a problem hiding this comment.
Critical Bug: tuple(kwargs) only extracts keyword argument NAMES, not their VALUES. This means func(1, a=3) and func(1, a=5) would have the same cache key and return wrong cached values. Use tuple(sorted(kwargs.items())) instead to include both keys and values in the cache key.
| curr_dict = cache_dict[func.__name__] | ||
| if curr_dict.get(tuple(args) + tuple(kwargs), None) is None: | ||
| print("Calculating new result") | ||
| curr_dict[tuple(args) + tuple(kwargs)] = func(*args, **kwargs) |
There was a problem hiding this comment.
Critical Bug: Same issue here - the cache key uses tuple(args) + tuple(kwargs) which ignores kwarg values. Use (*args,) + tuple(sorted(kwargs.items())) to properly include kwarg values.
|
Just in case, I'd like to know why the test fails. I've copied the test task to my file to test it closely, and the output was matching the test expectation. Still, the test was marked as failed for some reason. |
vsmutok
left a comment
vsmutok
left a comment
There was a problem hiding this comment.
Tests failed.
Please make sure your code passes the tests before submitting the task for review.
|
@vsmutok |
3 participants
No description provided.