Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed before this solution can be accepted because the current cache decorator breaks required functionality and causes tests to fail. In app/main.py, the wrapper is defined as def wrapper(*args) and never accepts or uses **kwargs, so calls with keyword arguments are not cached separately, which violates the requirement to handle different argument lists; update the signature to def wrapper(*args, **kwargs) and incorporate a deterministic representation of kwargs (e.g. tuple(sorted(kwargs.items())) or frozenset(kwargs.items())) into the cache key. The cache key currently uses (func.__name__, args) and a global cache, which can collide across different functions that share the same name and is the likely cause of test_cache_multiple_functions failing; instead, keep a separate cache per decorated function (define the dict inside cache) or include the function object itself in the key. Your cache lookup uses g_results.get(key) and then checks if result is None:, which means a real cached None result is treated as a miss; fix this by checking membership (if key in cache:) rather than comparing the retrieved value to None. Once these logic issues are fixed, also consider renaming g_results to a more descriptive name (e.g. _cache) and correcting the wrapper’s return type annotation so it reflects the function’s actual return type instead of Callable. Addressing these points will make your decorator robust for positional and keyword arguments, safe across multiple decorated functions, and aligned with the task checklist. Nice work on getting the basic caching and required print messages in place; with these adjustments, your solution should pass all tests.
✨ 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 | ||
| @functools.wraps(func) | ||
| def wrapper(*args) -> Callable: |
There was a problem hiding this comment.
The wrapper is defined as def wrapper(*args) and therefore does not accept keyword arguments. This will raise a TypeError if a decorated function is called with keywords and also prevents caching calls that differ only by keyword args. The task requires that the decorator stores results for runs with different argument lists (the description states: "number of arguments can also be different"). Consider changing the signature to def wrapper(*args, **kwargs) and include kwargs in the cache key (for example by using a tuple of sorted items or frozenset(kwargs.items())).
| result = g_results.get((func.__name__, args)) | ||
| if result is None: | ||
| result = func(*args) | ||
| g_results[(func.__name__, args)] = result |
There was a problem hiding this comment.
Using (func.__name__, args) as the cache key can cause collisions if multiple different functions have the same __name__. This violates the requirement that the decorator "should work correctly with a few decorated functions simultaneously and correctly return results for every function separately." Use the function object itself (e.g. func) or id(func) in the key to reliably distinguish functions. Also, the current logic checks if result is None: which means a legitimately cached value of None will be treated as a cache miss and recomputed. To fix both issues, check cache membership with if key in g_results: or use a sentinel instead of relying on None.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one of the required checklist items—using descriptive variable names—is currently violated.
The global cache dictionary g_results on line 4 of app/main.py is not self-explanatory, which conflicts with "Use descriptive and correct variable names" [CHECKLIST ITEM #4]; please rename it to something clearer like _cache, cache_store, or results_cache so its purpose is obvious at a glance. After renaming, make sure all references inside your decorator use the new name consistently so the cache continues to work as intended. Functionally, your decorator looks solid—it accepts *args/**kwargs, keys the cache by (func, args, kwargs), and prints the required "Calculating new result" and "Getting from cache" messages correctly.
As a minor improvement (not blocking), clean up the flake8 style issues in app/main.py by adding spaces after commas on lines 9 and 16 and removing trailing whitespace on lines 10 and 17, so your code passes the linter cleanly. Once you address the naming requirement and optional style fixes, your solution will be in great shape and aligned with both the functional and style expectations.
✨ 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
| from typing import Callable | ||
| import functools | ||
|
|
||
| g_results = {} |
There was a problem hiding this comment.
This violates the checklist item: "Use descriptive and correct variable names." The global name g_results (line 4) is not very descriptive. Consider renaming it to something clearer like _cache, cache_store, or results_cache so intent is immediately obvious.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because your cache decorator correctly stores results per function and argument set and returns cached values instead of recomputing them. This is an educational approval override to help you keep progressing while you address a couple of remaining polish points. You handled *args/**kwargs properly, included the function itself in the cache key so different decorated functions do not collide, and print exactly "Calculating new result" and "Getting from cache" as required [CHECKLIST ITEM #1]. One area to improve is the return type annotation on the wrapper in app/main.py (around the def wrapper(*args, **kwargs) line): it is annotated as returning Callable, but it actually returns the underlying function’s result, so consider adjusting the annotation (for example to a more general result type) or removing it to avoid confusion. Also, if this decorator might be used from multiple threads, note that the module-level cache_store dictionary is not thread-safe, so adding a simple lock around cache reads/writes would prevent race conditions. As a small style improvement, instead of checking if key in cache_store and then calling cache_store.get(key), you can directly index cache_store[key] inside that branch to avoid doing the lookup twice. Overall, your code is clean, readable, and meets the core requirements; nice work applying the checklist and naming the global cache descriptively [CHECKLIST ITEM #4].
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.