Implemented cache decorator #1905
Conversation
There was a problem hiding this comment.
Review Summary
This is a solid implementation of the caching decorator, but it needs a couple of adjustments. First, the wrapper function's return type hint is -> int, which is too specific. This prevents the decorator from being used on functions that return other types, like lists or strings. Please update it to a more generic type, like Any from the typing module. Second, the nonlocal cached_args line is not needed when you're modifying a mutable object like a dictionary. Its use here violates a specific project guideline [CHECKLIST ITEM #Clean Code.1], so please remove it. After these fixes, your solution will be perfect!
✨ 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
app/main.py
Outdated
| cached_args = {} | ||
|
|
||
| @wraps(func) | ||
| def wrapper(*args) -> int: |
There was a problem hiding this comment.
The return type hint -> int makes this decorator less reusable, as it implies the decorated function will always return an integer. According to the example in the description, long_time_func_2 returns a list. Consider using a more generic type hint, like typing.Any, or removing it to allow for any return type.
app/main.py
Outdated
|
|
||
| @wraps(func) | ||
| def wrapper(*args) -> int: | ||
| nonlocal cached_args |
There was a problem hiding this comment.
The nonlocal statement is not needed when you are modifying a mutable object like a dictionary, rather than reassigning the variable itself. Since you are just adding or accessing key-value pairs (cached_args[key] = res), you can safely remove this line. This violates checklist item #Clean Code.1.
app/main.py
Outdated
| @wraps(func) | ||
| def wrapper(*args) -> int: | ||
| nonlocal cached_args | ||
| key = (args, ) |
There was a problem hiding this comment.
The variable args is already a tuple containing all the positional arguments. You can use it directly as a dictionary key. By writing key = (args, ), you are creating a new tuple that contains the args tuple as its only element, for example ((1, 2, 3),). A simpler and more direct approach would be key = args.
There was a problem hiding this comment.
I added kwargs support, so it resolves automatically
- added kwargs support
There was a problem hiding this comment.
Review Summary
Great job removing the nonlocal statement! The core caching logic is solid. However, one critical issue remains before this can be approved.
In app/main.py, the inner wrapper function is still type-hinted to return an integer (-> int). This restricts your decorator to only work with functions that return integers, which doesn't meet the requirement for a general-purpose cache. To fix this, please import Any from the typing module and change the return type hint to -> Any. This will ensure your decorator works correctly with functions that return any type, such as lists or strings.
✨ 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
app/main.py
Outdated
| cached_args = {} | ||
|
|
||
| @wraps(func) | ||
| def wrapper(*args, **kwargs) -> int: |
There was a problem hiding this comment.
The return type hint -> int is too specific for a general-purpose cache decorator. This implementation won't correctly type-check for decorated functions that return other types, like lists or strings. Please change it to a more generic type, like Any from the typing module, which you'll need to import.
There was a problem hiding this comment.
Review Summary
Excellent progress on fixing the nonlocal issue! Your solution is very close, but this requires changes for two critical issues that affect the decorator's functionality.
-
Shared Cache: The cache is currently shared between all decorated functions. This means if you decorate two different functions, they will incorrectly use the same cache, leading to wrong results. Each function needs its own independent cache.
-
Incorrect Return Type Hint: The return type hint for the
wrapperfunction inapp/main.pyis still-> int. This prevents the decorator from working on functions that return other types, like a list. Please importAnyfrom thetypingmodule and use that as the return type.
Fixing these two issues will complete the task. Keep up the great work!
✨ 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
app/main.py
Outdated
| cached_args = {} | ||
|
|
||
| @wraps(func) | ||
| def wrapper(*args, **kwargs) -> int: |
There was a problem hiding this comment.
The return type hint -> int is too specific and hasn't been updated since the last review. This decorator should support functions that return any type, like the example long_time_func_2 which returns a list. Please change this to a more generic type, such as Any from the typing module (which you'll need to import).
There was a problem hiding this comment.
Review Summary
Excellent work on this revision! I am happy to approve your solution as you've successfully addressed all the previous feedback. The decorator now correctly provides a separate cache for each decorated function, and the return type hint is properly generalized using typing.Any. Your implementation is clean, efficient, and handles both positional and keyword arguments perfectly to create a stable cache key. This is a great example of a robust and well-designed decorator. Keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
3 participants
No description provided.