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

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

Solution #958

wants to merge 7 commits into from

Conversation

Ivan-Shakhman
Copy link

Its work XD

app/main.py Outdated

Choose a reason for hiding this comment

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

Don't commit nothing to main file

Choose a reason for hiding this comment

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

Delete DB from your PR

app/managers.py Outdated

def create(self, first_name: str, last_name: str) -> None:
self._connection.execute(
f"INSERT INTO {self.table_name} VALUES (?, ?, ?) ",

Choose a reason for hiding this comment

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

Use parametrization, here you now clear which fields you need to insert

app/managers.py Outdated
Comment on lines 26 to 28
f"UPDATE {self.table_name} "
f"SET first_name=?, last_name =? "
f"WHERE id=? ",

Choose a reason for hiding this comment

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

Use triple quotes

app/managers.py Outdated
f"UPDATE {self.table_name} "
f"SET first_name=?, last_name =? "
f"WHERE id=? ",
(f_name, l_name, key)

Choose a reason for hiding this comment

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

These are bad variable names

app/managers.py Outdated
)
self._connection.commit()

def all(self) -> list:

Choose a reason for hiding this comment

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

This functio will return list of what? Fix annotation

app/managers.py Outdated
Comment on lines 35 to 36
f"DELETE FROM {self.table_name} "
f"WHERE id=? ",

Choose a reason for hiding this comment

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

Use triple quotes

app/managers.py Outdated
self._connection.execute(
f"DELETE FROM {self.table_name} "
f"WHERE id=? ",
(key,)

Choose a reason for hiding this comment

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

Not key, but id)
You can put id_

app/main.py Outdated
@@ -11,3 +11,4 @@
# print(Actor.objects.all())
# Actor.objects.delete(1)

Choose a reason for hiding this comment

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

please revert changes to this file

Choose a reason for hiding this comment

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

still not done - make sure this file look exactly like on master

app/managers.py Outdated
@@ -0,0 +1,44 @@
import sqlite3
from models import Actor

Choose a reason for hiding this comment

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

needs to be a blank line between different kinds of imports + we use full path from app

app/managers.py Outdated
def __init__(self) -> None:
self._connection = sqlite3.connect("./cinema.sqlite")
self.table_name = "actors"
self.db_name = "cinema.sqlite"

Choose a reason for hiding this comment

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

why do you create this attribute and never use it?

app/managers.py Outdated
def create(self, first_name: str, last_name: str) -> None:
self._connection.execute(
f"""
INSERT INTO {self.table_name} (first_name, last_name) VALUES (?, ?)

Choose a reason for hiding this comment

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

Check sql formatting - each key word must start from a new line. Use this tool to help you with styling https://sqlformat.org/

app/main.py Outdated
@@ -11,3 +11,4 @@
# print(Actor.objects.all())
# Actor.objects.delete(1)

Choose a reason for hiding this comment

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

still not done - make sure this file look exactly like on master

app/managers.py Outdated
f"""
INSERT INTO {self.table_name} (first_name, last_name)
VALUES (?, ?)
""",

Choose a reason for hiding this comment

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

would be nice to move it just 4 spaces to the left :)

Copy link
Author

Choose a reason for hiding this comment

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

move what?

Copy link

@Oleksl888 Oleksl888 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!

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.

3 participants