Remove inline root, health, and echo routes#11
Conversation
Delete the local implementations of '/', '/health', and '/echo' from app/api/routes.py. These handlers were redundant after including root_router, health_router, and echo_router at the top of the file; removing them centralizes route definitions and eliminates the duplicate DB cursor and local Echo request/response logic.
There was a problem hiding this comment.
Pull request overview
This PR removes the inline implementations of the /, /health, and /echo endpoints from app/api/routes.py, relying instead on the already-included root_router, health_router, and echo_router to centralize route definitions and avoid duplication.
Changes:
- Deleted the local
GET /,GET /health, andPOST /echohandlers fromapp/api/routes.py. - Kept only the router composition via
include_router(...)for the three endpoint routers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| router.include_router(root_router) | ||
| router.include_router(health_router) | ||
| router.include_router(echo_router) |
There was a problem hiding this comment.
Removing the inline GET / handler changes runtime behavior to the implementation in app/api/root.py, which differs from the deleted one: it opens a new psycopg2 connection directly (bypassing get_db_connection's guaranteed cleanup), doesn’t close the cursor/connection in a finally, and casts price to float without a None-check (the removed handler returned None when row[3] is null and stringified non-null prices). To avoid a behavior regression and potential connection leaks, update the centralized root router to use the existing get_db_connection dependency and preserve the previous response shape/type handling (including safe cleanup on errors).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| router.include_router(root_router) | ||
| router.include_router(health_router) | ||
| router.include_router(echo_router) |
There was a problem hiding this comment.
After deleting the local route handlers, this module now appears to only build an APIRouter and include root_router, health_router, and echo_router. The remaining imports (__version__, os, time, psycopg2, load_dotenv, Depends, get_db_connection, EchoRequest, EchoResponse) are unused and may fail linting/CI; please remove them (and any now-unnecessary side-effect imports) to keep the module minimal.
|
@goldlabelapps I've opened a new pull request, #12, to work on those changes. Once the pull request is ready, I'll request review from you. |
[WIP] [WIP] Address feedback on removing inline root, health, and echo routes
Delete the local implementations of '/', '/health', and '/echo' from app/api/routes.py. These handlers were redundant after including root_router, health_router, and echo_router at the top of the file; removing them centralizes route definitions and eliminates the duplicate DB cursor and local Echo request/response logic.