Solution#1816
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because it fully implements the required Actor dataclass and ActorManager with all CRUD methods behaving as specified. Your Actor model correctly contains id, first_name, and last_name, and ActorManager takes db_name and table_name in __init__, sets up the SQLite connection there, and uses it in create, all, update, and delete as required. The all() method correctly returns a list of Actor instances (or an empty list when there are no rows), and update(pk, new_first_name, new_last_name) and delete(pk) operate on the id primary key as expected. Class names are in singular form and the table name is not hard-coded, which aligns with the checklist about naming and flexibility.
The only issue flagged in the checks is missing type annotations on parameters in app/managers.py (for example, db_name, table_name, first_name, last_name, pk, new_first_name, new_last_name), which is a style/linting concern rather than a functional problem. You can resolve this by annotating these arguments as str or int where appropriate, e.g. def create(self, first_name: str, last_name: str) -> Actor: and def delete(self, pk: int) -> None:. Since this does not break functionality or violate a core requirement, it does not block approval, but you should still address the annotations so that flake8-annotations passes. Overall, your implementation is clean, meets the task requirements, and only needs minor typing polish for full linting compliance.
✨ 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
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still one critical inconsistency in your implementation that affects the ActorManager API. In app/managers.py, the create method is annotated to return an Actor instance but currently does not return anything, so it effectively returns None, which contradicts both the type hint and the intended behavior of “creating a new entry and returning it.” This can mislead callers and may break type-checking or tests that rely on receiving an Actor object back from create.
To fix this, you should either:
- Update the implementation of
createto construct and return anActorwith the insertedid(e.g., usingcursor.lastrowid), or - Change the return type annotation to
-> Noneif the design is intentionally not to return anything.
Once this mismatch is resolved, the rest of your CRUD logic, dataclass definition, and connection handling look solid and aligned with the requirements. You’re very close; addressing this single issue will make your solution consistent and ready to move forward.
✨ 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._connection = sqlite3.connect(db_name) | ||
| self.table_name = table_name | ||
|
|
||
| def create(self, first_name: str, last_name: str) -> Actor: |
There was a problem hiding this comment.
create is annotated to return Actor but does not return anything, so it effectively returns None; align the implementation with the annotation by returning an Actor instance (e.g., with the inserted id) or change the return type to None if no value should be returned.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because it now fully matches the task requirements and the checklist for this exercise. You correctly defined the Actor dataclass in models.py with id, first_name, and last_name, and implemented ActorManager in managers.py with a proper __init__ that opens the SQLite connection using db_name and table_name. All CRUD methods (create, all, update, delete) have the right signatures and behave as expected: create inserts a record and returns an Actor with cursor.lastrowid, while all returns a list of Actor instances (or an empty list when there are no rows). This also aligns well with the checklist items such as singular class names [CHECKLIST ITEM #1] and using a configurable table_name instead of hardcoding it [CHECKLIST ITEM #2]. Overall, your implementation is clean, coherent, and demonstrates a solid understanding of basic ORM-like patterns with SQLite—well done.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.