work is done#1811
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a few critical issues that block the required functionality and tests. First, the task explicitly requires a create(self, first_name, last_name) method on ActorManager, but your class currently exposes create_actor instead, so any code/tests calling manager.create(...) will fail with an AttributeError. Second, in both update and delete, your SQL uses WHERE pk=? even though the dataclass and task description define the primary key as id; this mismatch will cause runtime errors or simply not update/delete the intended rows. Third, checklist expectations and the description assume plural table names for collections; while your manager accepts any table_name, you should ensure (or clearly rely on) a plural name like actors to comply with that convention [CHECKLIST ITEM #4].
To fix the critical blockers: 1) rename create_actor to create while keeping the same parameters and logic, so tests that call create will pass; 2) change your UPDATE and DELETE queries to use WHERE id=? instead of pk to align with the Actor.id field and typical SQLite schemas. As a minor improvement, double‑check that you are not committing .sqlite/.db files to the repo [CHECKLIST ITEM #2], and consider adding a way to close the DB connection cleanly when the manager is no longer needed. Once these changes are applied, your solution should fully meet the task requirements and behave correctly in the provided tests.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| self.table_name = table_name | ||
| self.connection = sqlite3.connect(self.db_name) | ||
|
|
||
| def create_actor(self, first_name: str, last_name: str) -> None: |
There was a problem hiding this comment.
The task description explicitly requires a method named create with parameters first_name and last_name; using create_actor instead means the required create method is missing and may break tests expecting ActorManager.create.
| def update(self, pk: int, new_first_name: str, new_last_name: str) -> None: | ||
| self.connection.execute( | ||
| f"UPDATE {self.table_name} " | ||
| f"SET first_name=?, last_name=? " | ||
| f"Where pk=?", |
There was a problem hiding this comment.
Your update query uses WHERE pk=?, but the dataclass and description define the unique identifier as id; using id instead of pk will better align with the schema and avoid runtime errors if the column is named id.
| def delete(self, pk: int) -> None: | ||
| self.connection.execute( | ||
| f"DELETE FROM {self.table_name} " | ||
| f"WHERE pk=?", |
There was a problem hiding this comment.
Similarly to update, delete targets pk as the column name; to stay consistent with the Actor.id field and likely DB schema, this should use id in the WHERE clause.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still a critical checklist violation: you have committed database files (identifier.sqlite and mydatabase.db) into the repository, which breaks the “no committed DB files” rule [CHECKLIST ITEM #1]. Functionally, your Actor dataclass and ActorManager implementation look solid: the dataclass fields are correct, the manager takes db_name and table_name in __init__, opens a sqlite3 connection, and implements create, all, update, and delete using id in the WHERE clause as required. To fix the blocking issue, remove those .sqlite/.db files from version control (e.g. git rm identifier.sqlite mydatabase.db), add an ignore rule (such as *.sqlite and *.db) to your .gitignore, and commit that change. Once those DB artifacts are no longer tracked, your solution will align with both the functional requirements and the checklist. Aside from that, you might optionally document that the table_name parameter should be plural (for example "actors") to match the naming convention, but that’s not a blocker. Overall, the core code is well structured; just clean up the committed DB files and you’ll be in great shape.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you learn, since the remaining issue is only a minor style point. You’ve correctly implemented the Actor dataclass with id, first_name, and last_name, and your ActorManager in managers.py implements create, all, update, and delete as required, using id as the primary key and properly returning an empty list when there are no rows. One small improvement: in app/managers.py inside your update method, the SQL string uses Where instead of WHERE; SQL is case-insensitive, but for readability and consistency with your delete method, you should change it to uppercase WHERE. This kind of consistency makes your code easier for others to read and aligns better with common SQL style.
Critical requirements are all met, and the only suggestion is this minor SQL keyword casing tweak, which you can fix quickly. Nice work implementing the required functionality cleanly and following the structure described in the task.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| self.connection.execute( | ||
| f"UPDATE {self.table_name} " | ||
| f"SET first_name=?, last_name=? " | ||
| f"Where id=?", |
There was a problem hiding this comment.
In this SQL statement you use Where with a lowercase h; SQL is case-insensitive so this works, but for readability and consistency with common style consider using WHERE like in your delete method.
2 participants
No description provided.