Skip to content
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

'help' #1625

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

'help' #1625

wants to merge 3 commits into from

Conversation

Dizatvarska
Copy link

No description provided.

Copy link

@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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

There are no files to review. Please make sure that you have added files to the pull request. Some files and directories may be ignored from the review or failed to load.
[CODE: 4]

Additional info:
Ignored files list:

  • app/main.py

If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.

Copy link

@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.

It looks like there are some critical issues that need to be addressed before we can approve this pull request. The Person class and the create_person_list function are not yet implemented, which are core requirements of the task. Additionally, a test file is missing, which is necessary to validate your implementation. Please implement these components and add tests to ensure your code meets the task requirements. Once these changes are made, your solution will be much closer to completion! 😊 Keep up the hard work, and don't hesitate to ask for help if you need it.

✨ 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.

app/main.py Outdated
@@ -5,4 +5,4 @@ class Person:

def create_person_list(people: list) -> list:
# write your code here
pass
pass

Choose a reason for hiding this comment

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

The create_person_list function is currently not implemented. You need to write the logic to create and return a list of Person objects based on the input people list. Ensure that you follow the task requirements for this implementation.

Copy link

@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

Great job on implementing the Person class and the create_person_list function! 🎉 Your solution meets the task requirements by defining the Person class with an __init__ method and maintaining a dictionary of instances. The set_relationships method is a nice touch for setting relationships between instances.

However, there are a few areas for improvement to ensure the robustness of your code:

  • Robustness in set_relationships Method: The method assumes that related persons already exist in the Person.people dictionary. It's important to add checks to ensure that the names exist before accessing them to prevent potential KeyError. This will make your code more resilient, especially with incomplete or incorrect input data.

  • Exception Handling in create_person_list: Consider handling exceptions when accessing the Person.people dictionary to further enhance the robustness of your function.

  • Testing: Currently, there is no test file provided. Adding tests using pytest will help validate your implementation and ensure it works as expected.

Despite these issues, the decision to approve your pull request is made to help you move forward with your learning. Please review the comments and address these issues to improve your code. Remember, addressing critical issues is essential to avoid similar problems in the future. Keep up the good work and continue learning from your experiences! 🚀

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


def set_relationships(self, people: dict) -> dict:
if "wife" in people and people["wife"]:
self.wife = Person.people[people["wife"]]

Choose a reason for hiding this comment

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

The code assumes that the wife's name exists in Person.people. Consider adding a check to ensure the name exists before accessing it to avoid potential KeyError.

del self.wife

if "husband" in people and people["husband"]:
self.husband = Person.people[people["husband"]]

Choose a reason for hiding this comment

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

Similar to the wife's check, ensure that the husband's name exists in Person.people before accessing it to prevent KeyError.

person_objects.append(person)

for person_data in people:
person = Person.people[person_data["name"]]

Choose a reason for hiding this comment

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

Ensure that the person's name exists in Person.people before accessing it to avoid potential KeyError. This is important for robustness, especially if the input data might be incomplete or incorrect.

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