Develop #4
Develop #4
Conversation
There was a problem hiding this comment.
Thank you for your hard work on the Movie Theater API project! 🎬 Your implementation is well-structured and mostly aligns with the task requirements. However, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Typo in
create_movieFunction: The variableexciting_movieshould be corrected toexisting_movieto ensure clarity and functionality when checking for existing movies. -
Inconsistent Pagination URLs: The pagination URLs in the
get_moviesfunction should be updated from/theater/movies/to/movies/to match the defined route and ensure consistency. -
Parameter Shadowing in Validator: In the
check_date_on_the_futurevalidator, the parameterdateshadows thedatemodule fromdatetime. Consider renaming this parameter to something likemovie_dateto avoid potential conflicts.
Once these issues are resolved, your code will be in great shape! Keep up the excellent work, and feel free to reach out if you have any questions or need further assistance. 😊
✨ 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.
src/routes/movies.py
Outdated
| MovieModel.name == movie.name, | ||
| MovieModel.date == movie.date | ||
| ).first() | ||
| if exciting_movie: |
There was a problem hiding this comment.
There is a typo here: exciting_movie should be existing_movie to correctly convey the meaning of checking if a movie already exists.
| prev_page = f"/theater/movies/?page={page - 1}&per_page={per_page}" | ||
| next_page = f"/theater/movies/?page={page + 1}&per_page={per_page}" |
There was a problem hiding this comment.
The pagination URLs should match the route defined for movies. Change /theater/movies/ to /movies/ to ensure consistency and correctness.
src/schemas/movies.py
Outdated
|
|
||
| @field_validator("date") | ||
| @classmethod | ||
| def check_date_on_the_future(cls, date: date): |
There was a problem hiding this comment.
The parameter date in the check_date_on_the_future method shadows the date module imported from datetime. Consider renaming this parameter to avoid conflicts, such as movie_date.
There was a problem hiding this comment.
Great job on implementing the movie routes in the FastAPI application! 🎉 Your code is mostly correct, but there are a few critical issues that need attention:
-
Inconsistent Pagination URLs: Ensure that the pagination URLs in the
get_moviesfunction are updated to/movies/instead of/theater/movies/to match the defined route and maintain consistency. -
Parameter Shadowing in Validator: The parameter
movie_datein theparse_and_validate_datevalidator shadows thedatemodule fromdatetime. Consider renaming this parameter to something likeinput_dateto avoid potential conflicts.
These issues are important for the functionality and clarity of your code. Please review the comments and address these points to improve your code. This decision is made to help you move forward with your learning. Keep up the good work and continue to refine your skills! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| prev_page = f"/theater/movies/?page={page - 1}&per_page={per_page}" | ||
| next_page = f"/theater/movies/?page={page + 1}&per_page={per_page}" |
There was a problem hiding this comment.
The pagination URLs should be updated to /movies/ instead of /theater/movies/ to match the defined route and ensure consistency.
| languages: list[str] | ||
|
|
||
| @field_validator("date") | ||
| def parse_and_validate_date(cls, movie_date: str | date): |
There was a problem hiding this comment.
The parameter movie_date in the parse_and_validate_date validator shadows the date module from datetime. Consider renaming this parameter to something like input_date to avoid potential conflicts.
No description provided.