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-py-shop-trip #406

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

Conversation

Nadiia-developer
Copy link

No description provided.

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.

Resolve flake8 errors for further assessment.

@Nadiia-developer
Copy link
Author

I resolved flake8 errors.

@Nadiia-developer
Copy link
Author

I don't understand why it is not possible to import: : ImportError: cannot import name 'Customer'

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 changes were requested.

app/shop/shop.py Outdated
import datetime
from typing import Dict, Any

from app.customer.customer import Customer
Copy link

@Dimosphen1 Dimosphen1 Oct 11, 2023

Choose a reason for hiding this comment

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

You cannot import Customer here because you import Shop inside of the app/customer/customer.py. You could read more about cyclic imports in Python and try to resolve them by reformatting the project structure upon the next pull request.

@Nadiia-developer
Copy link
Author

I have an issue with calculation.

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.

Resolve issues with tests and comments that are left in the PR upon the new review request.


def shop_trip() -> None:
with open("app/config.json", "r") as config_file:
config_data = json.load(config_file)

Choose a reason for hiding this comment

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

Do not overload the context manager, place logic not related to the file management outside of its scope.

Comment on lines +24 to +27
[
self.product_cart[product] * shop.products[product]
for product in self.product_cart
]

Choose a reason for hiding this comment

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

Do not use list comprehension inside of sum, use generator expression instead.

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