Solution#1829
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 a few critical issues that break the required behavior of your ActorManager.update method. First, the method signature must use the exact parameter names pk, new_first_name, and new_last_name; currently using first_name/last_name violates the task specification and can cause confusion when the function is called. Second, your SQL parameter binding is incorrect: the UPDATE statement expects (first_name, last_name, id) but you pass (pk, first_name, last_name), which will attempt to store the primary key in first_name and use last_name as the id, breaking updates and potentially causing runtime errors. Third, the SQL string literals for the UPDATE query are concatenated without a space before WHERE, producing something like ... = (?, ?)WHERE id = ?, which is invalid SQL and will fail at runtime. These three issues are functional blockers and must be fixed: rename the parameters to pk, new_first_name, new_last_name, pass them to the query as (new_first_name, new_last_name, pk), and ensure there is a space before WHERE in the final SQL string. Beyond that, consider ensuring table_name is plural as per the checklist and addressing the flake8 style warnings (return type annotations and trailing newlines), but those are secondary once the core functionality is correct. Overall, your structure is solid; with these fixes, your solution should fully meet the requirements.
✨ 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
| Actor(*row) for row in actors_cursor | ||
| ] | ||
|
|
||
| def update(self, pk: int, first_name: str, last_name: str): |
There was a problem hiding this comment.
The update method signature should match the task description: it must take pk, new_first_name, and new_last_name. Using first_name/last_name instead violates the requirement for these exact parameter names.
| self.db_name.execute( | ||
| f"UPDATE {self.table_name} " | ||
| "SET (first_name, last_name) = (?, ?)" | ||
| "WHERE id = ?", | ||
| (pk, first_name, last_name) |
There was a problem hiding this comment.
The UPDATE SQL sets (first_name, last_name) = (?, ?) and WHERE id = ?, so the parameter tuple must be (new_first_name, new_last_name, pk). Passing (pk, first_name, last_name) both breaks the mapping and violates the expected behavior for updating by pk.
| f"UPDATE {self.table_name} " | ||
| "SET (first_name, last_name) = (?, ?)" | ||
| "WHERE id = ?", |
There was a problem hiding this comment.
The SQL fragments for the UPDATE statement are concatenated string literals; as written, "SET ... = (?, ?)" and "WHERE id = ?" will join without a space, producing invalid SQL. Ensure there is a space before WHERE in either the end of the previous line or the start of this literal.
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, functionality-breaking issue in your ActorManager.update method. The SQL you build currently ends up as ... = (?, ?)WHERE id = ? with no space before WHERE, which will cause a runtime SQL error and prevents updates from working, directly violating the requirement that the update query include a space before WHERE. To fix this, ensure the SQL string has a space, for example by writing the SET clause and WHERE clause in a single string or adding a trailing space before concatenation, like:
"UPDATE " + self.table_name + " SET (first_name, last_name) = (?, ?) WHERE id = ?"Everything else looks structurally solid: your Actor dataclass is correct, the manager’s CRUD methods and signatures match the requirements, and you’re using the DB connection from __init__ appropriately. As a minor checklist point, double-check that your actual table name passed in at usage time is plural (e.g., "actors") and that you haven’t committed any .sqlite/.db3 files to the repo, but those are not blockers. Once you fix the SQL spacing in update, your solution should fully meet the task requirements.
✨ 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.
Review Summary
Your solution is approved. The Actor dataclass is correctly defined with id, first_name, and last_name, and your ActorManager cleanly encapsulates the SQLite connection and CRUD operations as required. The create, all, update, and delete methods follow the expected signatures and SQL behavior, including returning an empty list from all when there are no rows and correctly updating and deleting by id.
You also respected the checklist constraints, such as using a singular manager class name, allowing the table name to be injected externally (so it can be plural at the call site), and not committing any database files to the repository. As a minor improvement for future work, you might consider adding basic error handling or context managers around the database operations to make the code more robust, but this is not required for this task. Overall, this is a solid, clean implementation that meets the functional and checklist requirements—well done continuing to build good habits around structure and naming.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.