-
Notifications
You must be signed in to change notification settings - Fork 441
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
Develop #416
base: main
Are you sure you want to change the base?
Develop #416
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.
Great job on implementing the Library Management API! 🎉 Your code meets the task requirements and is well-structured. Just a few minor tweaks are needed: address the duplicate /authors/
route, and adjust the response models for create_author
and create_book
to reflect the full objects. Also, consider completing the Alembic migration scripts for a robust database setup. Keep up the excellent work! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
# ### commands auto generated by Alembic - please adjust! ### | ||
pass |
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 upgrade
function is currently empty. You should add the necessary operations to modify the database schema to fix the relationship between the author and book tables.
# ### commands auto generated by Alembic - please adjust! ### | ||
pass |
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 downgrade
function is also empty. Ensure to add the necessary operations to revert the changes made in the upgrade
function, allowing for a proper rollback if needed.
return {"Hello": "World"} | ||
|
||
|
||
@app.post("/create/author/", response_model=schemas.AuthorCreate) |
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 response model for create_author
should be schemas.Author
, not schemas.AuthorCreate
, to reflect the full author object after creation.
return crud.single_author(db=db, author_id=author_id) | ||
|
||
|
||
@app.post("/book/create/", response_model=schemas.BookCreate) |
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 response model for create_book
should be schemas.Book
, not schemas.BookCreate
, to reflect the full book object after creation.
return crud.create_book(db=db, book=book) | ||
|
||
|
||
@app.get("/authors/", response_model=List[schemas.Book]) |
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.
There is a duplicate route definition for /authors/
. Consider renaming this endpoint to avoid conflict with the read_authors
function.
main.py
Outdated
return crud.list_books(db=db, skip=skip, limit=limit) | ||
|
||
|
||
@app.get("/books/{author_id}", response_model=List[schemas.Book]) |
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.
@app.get("/books/{author_id}", response_model=List[schemas.Book]) | |
@app.get("/books/{author_id}/", response_model=List[schemas.Book]) |
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!
No description provided.