Implement ActorManager CRUD operations#1823
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 is still a checklist-related issue that counts as a critical requirement: your manager accepts any table_name and can be used with a singular name like "actor", which conflicts with the rule that tables containing multiple objects must be named in plural form [CHECKLIST ITEM #3]. To comply, you should ensure that the actual table you use is plural (for example "actors"), and it would help to either validate table_name in __init__ or clearly document that the expected value is plural so it cannot silently violate the checklist.
Functionally, your Actor dataclass and ActorManager implementation look solid: all CRUD methods are present and behave correctly, and all() will naturally return an empty list when there are no rows, which matches the task requirements. There are also linting warnings (missing type hints, long lines, newlines) that you will need to address to pass flake8, but those are style issues and can be fixed mechanically. Please adjust the table naming to guarantee plural usage and then clean up the linter errors; after that, your solution should fully meet both functional and checklist requirements. Overall, you’re very close—just tighten the table name convention and polish the style issues to finish this task successfully.
✨ 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
|
|
||
| def __init__(self, db_name, table_name): | ||
| self.db_name = db_name | ||
| self.table_name = table_name |
There was a problem hiding this comment.
This violates checklist item #3: "If db contains not only one object - name it plural. Good: self.table_name = \"objects\" Bad: self.table_name = \"object\"" — since table_name is fully dynamic, make sure the actual value you pass in is plural (e.g. "actors") rather than singular like "actor" to comply with the guideline.
|
Fix flake8 issues |
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 around table naming that must be fixed before this can be accepted. Your ActorManager currently takes table_name and uses it directly in SQL (e.g., INSERT INTO {self.table_name}), which means it can be instantiated with a singular name like "actor", violating the rule that tables holding multiple objects must be plural [CHECKLIST ITEM #2 / #3]. This is not just stylistic: the task explicitly requires plural naming for tables that store collections, so silently allowing singular names breaks the checklist requirements.
To fix this, add logic in ActorManager.__init__ (in app/managers.py, near the lines where self.table_name is set) that enforces or normalizes plural names. For example, you could: 1) ignore the passed table_name and always use a constant like "actors"; or 2) validate that table_name ends with "s" and raise a clear error if it does not; or 3) transform a singular input (like "actor") into a plural (like "actors") before assigning self.table_name. Any of these approaches would prevent the manager from working with a singular table name and bring you into full compliance.
Functionally, your CRUD implementation, dataclass definition, and database handling all look solid and meet the task’s behavioral requirements, so you are very close. Focus on tightening the table name handling so that the database table for multiple actors is guaranteed to be plural. Once that is done, your solution should satisfy both the functional description and the checklist. Keep up the good work—this is a small but important naming convention fix to complete the task.
✨ 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
| table_name: str, | ||
| ) -> None: | ||
| self.db_name = db_name | ||
| self.table_name = table_name |
There was a problem hiding this comment.
This violates checklist item #2: "If db contains not only one object - name it plural." Because self.table_name is taken directly from the parameter with no validation or normalization, ActorManager can be used with a singular table name like "actor". You should enforce or guarantee that the actual table name used here is plural (for example, by validating the argument, normalizing it to a known plural like "actors", or otherwise preventing singular names).
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 and ActorManager implementation clearly meet the described CRUD and checklist requirements. The dataclass correctly models id, first_name, and last_name, and your manager cleanly handles create, all, update, and delete using a SQLite connection initialized in __init__. Normalizing the table_name to a plural form inside __init__ is a thoughtful way to respect the plural-table naming rule while keeping the class name singular, which aligns well with the checklist expectations.
You’ve done a nice job keeping the logic straightforward and readable, and your methods return and manipulate Actor instances in a consistent way. Overall, this is solid, idiomatic Python and a good example of separating data models from database management logic. Keep building on this structure as you add more models and managers — you’re on the right track.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
Completed: