Solution#1831
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is a critical mismatch between the database row ordering and the Actor dataclass field order, which breaks the core functionality of returning correct Actor instances. Right now, the dataclass in app/models.py is defined as first_name, last_name, id, but ActorManager.all in app/managers.py does SELECT * and then calls Actor(*row), which for a typical schema (id, first_name, last_name) assigns values to the wrong attributes (e.g., id ends up in first_name). This directly violates the requirement that all must return correct Actor objects and is also causing the unit tests to fail.
The main fix you need is to reorder the fields in the Actor dataclass to id, then first_name, then last_name, so that Actor(*row) maps correctly when the row is (id, first_name, last_name). Alternatively, you could explicitly pass keyword arguments like Actor(id=row[0], first_name=row[1], last_name=row[2]), but the simplest way that also matches the textual requirement is to put id first in the dataclass. Once that mapping is corrected, your existing CRUD methods (create, all, update, delete) and the behavior of all returning an empty list when there are no rows will satisfy the task. As a minor improvement, you can also adjust type hints to follow PEP 8 (first_name: str instead of first_name : str), but this is not a blocker. Overall, your structure is solid; fixing the Actor field order will bring the solution in line with the requirements and make the tests pass.
✨ 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
|
|
||
| # add manager here | ||
| class ActorManager: | ||
|
|
||
| def __init__(self, db_name: str, table_name: str) -> None: |
There was a problem hiding this comment.
The Actor dataclass fields are declared as first_name, last_name, id, but the requirements list attributes as id, first_name, last_name, and your all method unpacks rows directly (Actor(*row)), which will misalign values if the DB schema is id, first_name, last_name. Reorder the fields to id, then first_name, then last_name to satisfy the description and keep the unpacking correct.
| class Actor: | ||
| first_name : str | ||
| last_name : str | ||
| id : int |
There was a problem hiding this comment.
The Actor dataclass currently defines fields in the order first_name, last_name, id, but ActorManager.all uses Actor(*row) where the row from SELECT * will typically be (id, first_name, last_name). This means the values are assigned to the wrong attributes, breaking the requirement that all returns correct Actor instances. Consider placing id first in the dataclass or adjusting how you build Actor from the query result.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it fully meets the functional and checklist requirements for this task. The Actor dataclass in models.py is correctly defined with fields id, first_name, last_name in the proper order, so Actor(*row) now maps database rows correctly. The ActorManager in managers.py has the required __init__(self, db_name, table_name) signature, creates the SQLite connection in the constructor, and correctly stores both parameters. All CRUD methods (create, all, update, delete) are implemented with the specified signatures, operate on the configured table_name, and commit changes appropriately, so normal usage and tests should work as intended.
The only remaining observation is a minor style point: type annotations in Actor could follow PEP 8 spacing conventions (e.g., first_name: str instead of first_name : str), but this is purely cosmetic and not required for this task. Overall, you’ve addressed the earlier critical issue and produced a clean, functional implementation—well done.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.