-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement CRUD for Movies #40
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your hard work on this pull request! While your implementation for the movie deletion and update endpoints is mostly on point, there are a few critical issues that need to be addressed before we can proceed:
-
Incorrect Import Statement: The import statement for
ceil
is incorrect. Please ensure it readsfrom math import ceil
. -
Pagination Logic: There are several issues with pagination logic:
- The condition
if end >= items:
should beif end > items:
to correctly determine if there are more pages. - The calculation of
total_pages
should useceil(len(data) / per_page)
to ensure the total number of pages is rounded up correctly. - The condition
if not items or page > total_pages:
might not work as expected. Consider checking ifpage
is greater thantotal_pages
separately.
- The condition
-
Error Handling in
update_movie
:- Catching
ValueError
might not be appropriate. Consider using a more specific exception likeSQLAlchemyError
. - Setting
existing_movie.detail
is likely incorrect asMovieModel
probably doesn't have adetail
attribute.
- Catching
-
Pydantic Validators:
- Replace
@field_validator
with@validator
in your Pydantic schemas. - The default value for
date
inMovieUpdateSchema
should beNone
instead ofdate
.
- Replace
These changes are crucial to ensure the functionality works as intended and aligns with best practices. Once these are addressed, your code will be in great shape! Keep up the great work and don't hesitate to reach out if you need any help. 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review
button in the Reviewers tab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great effort on your Pull Request! 🎉 While the decision is to APPROVE to help you move forward with your learning, there are several important issues that need your attention:
-
Import Statement for
ceil
: Ensure the import statement isfrom math import ceil
to avoid syntax errors. -
Pagination Logic:
- Adjust the condition to
if end > items:
for correct page determination. - Use
ceil(len(data) / per_page)
for calculatingtotal_pages
to ensure proper rounding. - Re-evaluate the condition
if not items or page > total_pages:
to ensure it functions as expected.
- Adjust the condition to
-
Error Handling in
update_movie
:- Replace
ValueError
with a more specific exception likeSQLAlchemyError
for better error handling. - Verify that you are updating the correct attributes of
MovieModel
, asexisting_movie.detail
might not exist.
- Replace
-
Pydantic Validators:
- Replace
@field_validator
with@validator
in your Pydantic schemas. - Ensure optional fields, like
date
inMovieUpdateSchema
, default toNone
.
- Replace
These changes are crucial for the functionality and best practices of your code. Address these issues to enhance your solution and learn from this experience. Keep up the good work and continue improving! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -1,12 +1,184 @@ | |||
from math import ceil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statement for ceil
is incorrect. It should be from math import ceil
instead of from math import ceil /
.
except ValueError: | ||
db.rollback() | ||
existing_movie.detail = "Invalid input data." | ||
raise HTTPException(status_code=400, detail="Invalid input data.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching ValueError
might not be appropriate here. Consider using a more specific exception like SQLAlchemyError
for database operations.
|
||
except ValueError: | ||
db.rollback() | ||
existing_movie.detail = "Invalid input data." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting existing_movie.detail
is likely incorrect as MovieModel
probably doesn't have a detail
attribute. Ensure that you are updating the correct attributes of the MovieModel
.
from re import match | ||
|
||
from dateutil.relativedelta import relativedelta | ||
from pydantic import BaseModel, field_validator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace @field_validator
with @validator
in your Pydantic schemas. field_validator
is not a valid decorator in Pydantic.
|
||
class MovieUpdateSchema(MovieBase): | ||
name: Optional[str] = None | ||
date: Optional[date] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value for date
in MovieUpdateSchema
should be None
instead of date
. Ensure that optional fields are correctly handled.
No description provided.