Solve task about mutable/immutable types#3461
Solve task about mutable/immutable types#3461Khripkov-Ihor wants to merge 2 commits intomate-academy:masterfrom
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the key requirement of exposing a module-level sorted_variables dictionary is not yet met, which will cause the tests to fail. Right now, sorted_variables is created only inside the sortable function in app/main.py, and that function is never called, so there is no top-level sorted_variables variable with the required structure. You need to define a module-level dictionary named sorted_variables with exactly two keys, "mutable" and "immutable", where the lists contain all 8 existing module variables grouped by mutability (mutable: my_favourite_films, marks, collection_of_coins; immutable: lucky_number, pi, one_is_a_prime_number, name, profile_info). You can either build this dictionary directly at the top level or call your function with those eight variables and assign its return value to sorted_variables, but it must exist at module scope. In addition, flake8 reports several style issues in app/main.py (around line 20: missing blank line, extra space before (, missing return type annotation, spacing after :, and incorrect indentation) that you should fix so the linter passes. As a minor improvement, prefer isinstance(i, (list, dict, set)) over using a set of types and type(i) in ..., since it is more idiomatic and handles subclasses correctly. You’ve correctly defined the eight variables with appropriate types, so once you expose sorted_variables at the module level and clean up the formatting, your solution should be in good shape.
✨ 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 | ||
| def sortable (*args): | ||
| sorted_variables = {"mutable":[] , "immutable":[]} |
There was a problem hiding this comment.
The task requires a module-level variable named sorted_variables (containing two lists with all 8 module variables). Here sorted_variables is created inside the function (local scope), so it won't be available at module level. Move creation/population of sorted_variables to the top level so tests can access it.
| collection_of_coins = {1, 2, 25} | ||
|
|
||
| # write your code here | ||
| def sortable (*args): |
There was a problem hiding this comment.
Defining sortable alone doesn't satisfy the requirement because the function is never called and no top-level sorted_variables is assigned. Either remove the function and define sorted_variables directly, or call sortable(...) with the eight variables and assign its return value to a module-level sorted_variables.
| # write your code here | ||
| def sortable (*args): | ||
| sorted_variables = {"mutable":[] , "immutable":[]} | ||
| mutable = {list, dict, set} |
There was a problem hiding this comment.
Using mutable = {list, dict, set} and type(i) in mutable works for direct types but is less robust. Prefer isinstance(i, (list, dict, set)) to correctly handle subclasses and be more idiomatic.
| for i in args: | ||
| if type(i) in mutable: | ||
| sorted_variables["mutable"].append(i) | ||
| else: | ||
| sorted_variables["immutable"].append(i) | ||
| return sorted_variables |
There was a problem hiding this comment.
The loop processes args, but since the function is never invoked, this code never runs. Ensure you either call sortable(lucky_number, pi, one_is_a_prime_number, name, my_favourite_films, profile_info, marks, collection_of_coins) and assign its result to module-level sorted_variables, or directly construct sorted_variables at module level with the correct grouping.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it correctly builds the sorted_variables dictionary at module level and groups all eight variables into the required "mutable" and "immutable" lists with the right contents.
The remaining issues are stylistic rather than functional. In app/main.py, before the top-level function definition def sortable(...):, you need one additional blank line so that there are two blank lines separating it from the preceding code, as required by PEP8; this will satisfy flake8’s spacing rule. Also in app/main.py, where you currently use type(i) in {list, dict, set} to detect mutability, consider switching to isinstance(i, (list, dict, set)), which is more idiomatic and correctly handles subclasses of these types. These adjustments will improve style and linter compliance but do not affect the correctness of your current solution.
Overall, you met the core functional requirements well; treat the spacing fix and isinstance change as good practice for writing more maintainable, idiomatic Python.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| # write your code here | ||
|
|
||
| def sortable(*args) -> dict: |
There was a problem hiding this comment.
PEP8/flake8: top-level functions must be separated from preceding code by two blank lines. Add an extra blank line before this function definition so the linter stops reporting a missing blank line.
| sorted_variables = {"mutable": [], "immutable": []} | ||
| mutable = {list, dict, set} | ||
| for i in args: | ||
| if type(i) in mutable: |
There was a problem hiding this comment.
Style/improvement: using type(i) in {list, dict, set} is brittle (it misses subclasses). Prefer isinstance(i, (list, dict, set)), which is more idiomatic and handles subclasses correctly.
No description provided.