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 #1242

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

Solution #1242

wants to merge 3 commits into from

Conversation

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

Great work so far! 😊 However, there are a few areas that need some improvement. Please, pay attention to the overridden __init__ method in the FullStackDeveloper class and the usage of append method. Once these changes are made, your code will be perfect! Keep going! 💪

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

app/main.py Outdated
Comment on lines 9 to 11
def learn_skill(self, skill: str) -> None:
if skill not in self.skills:
self.skills.append(skill)

Choose a reason for hiding this comment

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

You're using append method to add a single value to a list (checklist point 1). It's not an error, but using extend() or += with a list of one element would be more consistent with the rest of your code.

app/main.py Outdated
Comment on lines 45 to 46
def __init__(self, name: str) -> None:
super().__init__(name)

Choose a reason for hiding this comment

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

You're overriding the __init__ method in the FullStackDeveloper class without adding any additional logic (checklist point 3). If you're not changing the behavior of the method, there's no need to override it.

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.

Great job on your code! 👍 However, there's a small issue with the FullStackDeveloper class constructor. You're overriding it without adding any new logic. This is not necessary and can be removed to make your code cleaner. Keep up the good work and happy coding! 😊

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

app/main.py Outdated
Comment on lines 45 to 46
def __init__(self, name: str) -> None:
super().__init__(name)

Choose a reason for hiding this comment

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

In the FullStackDeveloper class, you are overriding the __init__ method, but you are not adding any additional logic inside. This is a bad practice according to the checklist. You should only override a method if you add additional logic inside. In this case, you can remove the __init__ method from the FullStackDeveloper class as it doesn't add anything new to the class.

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.

Great job on your code! It is well-structured and follows the checklist requirements. However, there is a minor issue with the create_web_application method in the FullStackDeveloper class. It's overridden without adding any unique logic. This is not a good practice and can lead to confusion. Consider either removing it or adding some unique logic. Keep up the good work and happy coding! 🚀

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

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