Skip to content

Solution#2189

Open
jlaourayed wants to merge 4 commits intomate-academy:masterfrom
jlaourayed:develop
Open

Solution#2189
jlaourayed wants to merge 4 commits intomate-academy:masterfrom
jlaourayed:develop

Conversation

@jlaourayed
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there is a functional gap in the caching behavior and a direct checklist violation that both need to be addressed before this can move forward. In app/main.py, inside the cache decorator where you build the cache key, using tuple(kwargs.items()) means calls like func(a=1, b=2) and func(b=2, a=1) will generate different keys and miss the cache; you should construct the kwargs part of the key with a deterministic ordering, e.g. based on sorted(kwargs.items()), so logically identical calls share the same cached result. In the same file, within the decorator’s closure (near the definition of the cache dictionary and key), variable names such as resultats_executions and cle are not clear English identifiers and violate [CHECKLIST ITEM #4]; please rename them to something descriptive like cache_store (or results_cache) and key so the intent is immediately obvious. These two changes are required because they directly affect correctness (consistent caching for equivalent arguments) and adherence to the explicit checklist you were asked to follow. As minor improvements (not blockers), consider wrapping the inner function with functools.wraps(func) to preserve metadata and adjusting the wrapper’s return type annotation to something like -> Any or omitting it, since -> func is misleading. Once you fix the cache-key construction and variable names, your decorator will align well with the task requirements and be a solid, readable solution.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread app/main.py Outdated
Comment on lines +5 to +8
resultats_executions = {}

def wrapper(*args, **kwargs) -> func:
cle = (args, tuple(kwargs.items()))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item: "Use descriptive and correct variable names." The variables resultats_executions and cle are not descriptive English names; prefer clearer names like cache_store or results_cache and key to improve readability and follow the checklist.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oui bien vu

Comment thread app/main.py Outdated
resultats_executions = {}

def wrapper(*args, **kwargs) -> func:
cle = (args, tuple(kwargs.items()))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructing the cache key as tuple(kwargs.items()) can cause cache misses if the same keyword arguments are passed in a different order. Use a deterministic ordering such as tuple(sorted(kwargs.items())) so that keyword arguments {a=1, b=2} and {b=2, a=1} produce the same key.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oui bien vu

@jlaourayed
Copy link
Copy Markdown
Author

j'ai corrigé le tri des kwargs

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because one explicit checklist requirement is still violated, even though the core caching behavior is correct and working. In app/main.py, the variables resultats_executions and cle (near the decorator’s cache dictionary and key-construction logic) are not descriptive English names, which breaks “Use descriptive and correct variable names” [CHECKLIST ITEM #1]; please rename them to something clearer like results_cache (or cache_store) and key so their intent is obvious. This change is required before the solution can be considered complete, since checklist items are part of the task’s functional requirements.

Separately, there is a minor improvement you can make: the wrapper’s return annotation -> func is misleading because it refers to the function object, not the return value type; consider removing the annotation or using a more accurate type such as -> Any. The caching logic itself (per-function cache, deterministic key for kwargs, and correct printing of cache vs new computation) looks good. Once you fix the variable names and optionally adjust the return annotation, your decorator will both meet the functional goals and align with the style checklist—nice work getting the behavior right so far.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread app/main.py Outdated
Comment on lines +6 to +11
resultats_executions = {}

@wraps(func)
def wrapper(*args, **kwargs) -> func:
sorted_kwargs = tuple(sorted(kwargs.items()))
cle = (args, sorted_kwargs)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates the checklist item "Use descriptive and correct variable names." The variables resultats_executions and cle are not descriptive English names. Rename them to something clear like results_cache (or cache_store) and key so the intent is obvious and matches the checklist.

Comment thread app/main.py Outdated
resultats_executions = {}

@wraps(func)
def wrapper(*args, **kwargs) -> func:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional/low-severity: the wrapper's return annotation -> func is misleading (it references the function object, not the return value type). Consider using -> Any (from typing) or omitting the return annotation.

@jlaourayed
Copy link
Copy Markdown
Author

variables renommées en Anglais

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution because it correctly implements the caching decorator’s behavior: it keeps a per-function cache, supports both *args and **kwargs, and prints different messages for cached versus newly computed results. Your variable naming and structure are clear and readable (for example, using results_cache and key), which aligns well with [CHECKLIST ITEM #1] about descriptive names. There is one minor issue in app/main.py: on the line where you define the wrapper (def wrapper(... ) -> func:), the return annotation -> func is misleading, since func is the decorated function object, not the return type, and this can confuse readers and type checkers. A good improvement would be to either remove that return annotation or change it to something like -> Any and import Any from typing so that the annotation reflects that the wrapper returns whatever the original function returns. Because this is a small style/type-hint concern and your core logic is correct, this is an educational override approval to help you keep progressing while you refine details like type hints. Overall, you’ve done a solid job on the functionality—treat this type-hint tweak as a chance to polish already strong work.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread app/main.py Outdated
results_cache = {}

@wraps(func)
def wrapper(*args, **kwargs) -> func:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wrapper's return annotation -> func is misleading: func refers to the function object, not the return value type. Consider removing the annotation or using -> Any and importing Any from typing for clarity.

@jlaourayed
Copy link
Copy Markdown
Author

"Type: change wrapper return annotation to Any for clarity

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.

2 participants