Skip to content

Solution#2965

Open
akydyraliuulu wants to merge 2 commits into
mate-academy:masterfrom
akydyraliuulu:develop
Open

Solution#2965
akydyraliuulu wants to merge 2 commits into
mate-academy:masterfrom
akydyraliuulu:develop

Conversation

@akydyraliuulu

Copy link
Copy Markdown

No description provided.

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

Copy link
Copy Markdown

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 risk with the ordering of the returned list and a direct conflict with an explicit checklist requirement. In app/main.py, create_person_list currently returns list(Person.people.values()), which does not guarantee that person_list[0] corresponds to the first dict in the input list; this can break tests and violates the requirement that the output list preserves input order. To fix this, you should build the result list by iterating over the original people list in order, e.g., by collecting Person instances corresponding to each input dict in that same sequence. Additionally, [CHECKLIST ITEM #3] explicitly asks you to use a list comprehension when creating a list of instances from a collection; updating create_person_list to construct the result list via a comprehension over people will satisfy this requirement. Aside from this, your Person class design, spouse-linking logic, and conditional spouse attributes are all implemented correctly and align well with the task description. Once you preserve the input order and adopt the list comprehension as described, your solution will fully meet the functional and checklist 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread app/main.py Outdated
current_person.wife = Person.people[person["wife"]]
if person.get("husband") is not None:
current_person.husband = Person.people[person["husband"]]
return list(Person.people.values())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Returning list(Person.people.values()) can break the required positional behavior (e.g., person_list[0] should correspond to the first dict in the input). This also conflicts with checklist item #3: When creating a list of instances from a collection (such as a list of dictionaries), it is recommended to use list comprehension rather than traditional for loops. Consider constructing and returning the list in the same order as the input people list, ideally via a list comprehension.

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

Copy link
Copy Markdown

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 fully meets the functional requirements and the checklist constraints for this task. Your Person class correctly initializes name and age, and registers each instance in the class-level Person.people dictionary keyed by name, which enables proper cross-linking. The create_person_list function uses a list comprehension to build the list of Person instances in the original input order, and then cleanly sets wife and husband attributes only when the corresponding values are not None. By using person.get("wife") and person.get("husband"), you follow the required dictionary access pattern and avoid unnecessary key checks. The behavior when a spouse is missing (raising AttributeError because the attribute is not set) aligns with the expected example. Overall, the code is clear, respects the checklist items, and implements all required features without any blocking issues.


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

Footnotes

  1. Rate AI review example

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