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

Solution #879

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

Solution #879

wants to merge 6 commits into from

Conversation

DanuloM
Copy link

@DanuloM DanuloM commented Dec 8, 2023

No description provided.

app/main.py Outdated
for person in people:
person1 = Person(person["name"], person["age"])

if "wife" in person and person["wife"]:

Choose a reason for hiding this comment

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

use get() instead of two conditions

app/main.py Outdated
person1.wife = Person.people[person["name"]]
Person.people[person["name"]].husband = person1

if "husband" in person and person["husband"]:

Choose a reason for hiding this comment

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

use get() instead of two conditions

README.md Outdated
and store `name`, `age` of a person.
This class also should have a class attribute
`people`, it is a dict that stores `Person`
instances by their `name`. Constructor should
instances by their `name`. The `__init__` method should

Choose a reason for hiding this comment

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

why did you change this file?

Copy link
Author

@DanuloM DanuloM Dec 10, 2023

Choose a reason for hiding this comment

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

i didnt change that. I think it it was IDE when i set up this project. should i fix this?

Choose a reason for hiding this comment

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

Yes, remove this file from the PR, please. Only app/main.py should be present.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should rollback all changes that you have made in README.md. Your Readme must be the same as on main branch

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use git checkout -p master -- README.md. But make sure that you are on develop branch

@DanuloM DanuloM requested a review from pavlejviki December 10, 2023 22:59
Copy link

@Dimosphen1 Dimosphen1 left a comment

Choose a reason for hiding this comment

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

Several minor changes were requested.

README.md Outdated
and store `name`, `age` of a person.
This class also should have a class attribute
`people`, it is a dict that stores `Person`
instances by their `name`. Constructor should
instances by their `name`. The `__init__` method should

Choose a reason for hiding this comment

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

Yes, remove this file from the PR, please. Only app/main.py should be present.

@DanuloM DanuloM requested a review from Dimosphen1 December 11, 2023 18:44
Copy link

@Dimosphen1 Dimosphen1 left a comment

Choose a reason for hiding this comment

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

You should not remove the README.md file from the repository, you should not apply any changes to it. Recreate it with the content from here and make a new commit to resolve the conflicts and run tests.

@DanuloM DanuloM requested a review from Dimosphen1 December 12, 2023 15:57
@DanuloM
Copy link
Author

DanuloM commented Dec 12, 2023

created file README.md, clicked on here and added the text.

README.md Outdated
and store `name`, `age` of a person.
This class also should have a class attribute
`people`, it is a dict that stores `Person`
instances by their `name`. Constructor should
instances by their `name`. The `__init__` method should
Copy link
Contributor

Choose a reason for hiding this comment

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

You should rollback all changes that you have made in README.md. Your Readme must be the same as on main branch

@DanuloM
Copy link
Author

DanuloM commented Dec 12, 2023

image
this is screenshot of readme file from main branch, i have the same. But ok if i want to revert all the changes from readme file how can i do that for one single file?

app/main.py Outdated
# write your code here
pass

people_list = []

Choose a reason for hiding this comment

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

In this variable first of all you should create list of Person objects, instead of doing this in regular cycle

@DanuloM DanuloM requested a review from pavlopro August 15, 2024 13:03
Copy link

@LLkaia LLkaia left a comment

Choose a reason for hiding this comment

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

In general, looks good. Added comment for consideration.

people_list = [Person(person["name"], person["age"]) for person in people]

for person in people:
person1 = Person.people[person["name"]]
Copy link

Choose a reason for hiding this comment

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

This is not another person to name it person1, it's just an object of person. So, to name variable more specific, better to name it person_object or smth like that.

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.

6 participants