-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
decorator #1379
base: master
Are you sure you want to change the base?
decorator #1379
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! 😊 It looks like there are a few critical issues that need to be addressed before we can approve the changes. The most important one is that the wrapper
function does not return the result of the wrapped function, which is essential for the decorator to work correctly. Additionally, the caching logic needs to be improved to store results based on the arguments passed to the function, ensuring it works correctly with multiple decorated functions. Once these issues are resolved, your solution will be much closer to meeting the task requirements. Keep up the good work, and don't hesitate to ask for help if needed! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review
button in the Reviewers tab.
print("Calculating new result") | ||
else: | ||
print("Getting from cache") | ||
return wrapper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wrapper
function should return the result
of the wrapped function func
. Without returning the result, the decorated function will not produce any output, which is likely not the intended behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your submission! 🎉 While your code currently has some issues, such as the incorrect implementation of the caching mechanism, it's important to recognize your effort and encourage you to keep improving. Here are some key points to address:
-
Cache Implementation: The current caching logic only stores results without associating them with their input arguments. This is a critical flaw, as it cannot differentiate between results from different inputs. Consider using a dictionary to map input arguments to their results.
-
Argument-Based Caching: Ensure that the decorator stores results based on the arguments, allowing it to return stored results for repeating arguments.
-
Output Messages: The decorator should print "Calculating new result" or "Getting from cache" instead of returning these strings.
-
Multiple Decorated Functions: Ensure that the
cache
decorator can handle multiple decorated functions simultaneously. -
Immutable Arguments: Although not explicitly checked, remember that the decorator is intended for functions with immutable arguments.
These improvements will help you meet the task requirements and enhance the functionality of your code. Keep up the good work, and use this feedback to learn and grow as a developer! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
||
|
||
def cache(func: Callable) -> Callable: | ||
# Write your code here | ||
pass | ||
a = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list a
is used to store results, but it does not associate results with their input arguments. This will cause incorrect behavior if different inputs produce the same result, as the cache will incorrectly return 'Getting from cache' for different inputs that happen to produce the same result.
@wraps(func) | ||
def wrapper(*args, **kwargs) -> any: | ||
result = func(*args, **kwargs) | ||
if result not in a: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caching logic is flawed because it only checks if the result is in the list a
, without considering the input arguments. This means the cache cannot differentiate between results from different inputs.
def wrapper(*args, **kwargs) -> any: | ||
result = func(*args, **kwargs) | ||
if result not in a: | ||
a.append(result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appending only the result to the cache list a
is insufficient for a proper caching mechanism. You need to store the result associated with its input arguments, typically using a dictionary where the key is the arguments and the value is the result.
No description provided.