Add health-check, operations (skeleton) endpoints and unit tests #3
Conversation
|
We need to fix this:
It should run either for push or pull, not booth of them. |
|
Also, the project configs are not consistent. Make sure to update them and run the CI locally to check if it is passing.
|
| @router.get("/health", tags=["Health"]) | ||
| async def health_check(): | ||
| """Simple health-check endpoint""" | ||
| return {"status": "ok", "version": "v1"} |
There was a problem hiding this comment.
What does "version" here represent?
There was a problem hiding this comment.
I think we can remove the version from this endpoint.
| @router.get("/health", tags=["Health"]) | ||
| async def health_check(): | ||
| """Simple health-check endpoint""" | ||
| return {"status": "ok", "version": "v1"} |
There was a problem hiding this comment.
I think we can remove the version from this endpoint.
|
|
||
| @router.get("/health", tags=["Health"]) | ||
| async def health_check(): | ||
| """Simple health-check endpoint""" |
| ) | ||
|
|
||
| app.include_router(health_router, prefix="/api") | ||
| app.include_router(operations_router, prefix="/api/v1") |
There was a problem hiding this comment.
Make sure to add a newline at the end of each file. @IbraheemTuffaha the template should implement such thing.
| @@ -0,0 +1,16 @@ | |||
| from fastapi import APIRouter, HTTPException, status | |||
|
|
|||
| from app.v1.models.operations_request import OperationsRequest | |||
There was a problem hiding this comment.
You can simplify the imports by writing:
from app.v1.models import OperationRequestIf you add a file named __init__.py under app/v1/models and put the following inside it:
from app.v1.models.operation_request import OperationRequest
__all__ = ['OperationRequest']This way you will have more simpler imports over the app.
| from app.v1.models.operations_response import OperationsResponse | ||
| router = APIRouter() |
There was a problem hiding this comment.
Make sure to split imports from global variables by 2 lines. @IbraheemTuffaha the template should handle such thing.
| surah_number: int = Field(..., title="Surah Number", | ||
| description="Number of the Surah in the Quran.") | ||
| ayah_start: int = Field(..., title="Starting Ayah", | ||
| description="Ayah (verse) number where the match starts in the Surah.") | ||
| description="Ayah (ayah) number where the match starts in the Surah.") |
There was a problem hiding this comment.
(ayah) is duplicated, and the same comment from Correction model, I don't see a huge value from the title and description tbh.
Same applies for other models/attributes.
| min_match: int = Field(3, title="Min Match Length", | ||
| description="Minimum word count for a valid match.") | ||
| description="Minimum word count for a valid match.") | ||
| return_json: bool = Field(False, title="Return JSON", |
There was a problem hiding this comment.
Sorry if I missed this from the design doc, but what is the difference between passing/not passing this argument? And why not always return the structured JSON?
There was a problem hiding this comment.
I wanted to maintain the output format to be same output as in original package.
There was a problem hiding this comment.
I think the API is a special case, so let's return JSON always as it is the standard for API responses.
| matches: Optional[list[AyahMatch]] = Field( | ||
| None, title="Detected Ayaht", |
| from app.v1.models.correction import Correction | ||
|
|
||
|
|
||
| def test_valid_correction(): |
There was a problem hiding this comment.
Why not used correction_dict from conftest.py? tbh, I don't feel that having correction_dict and ayah_match_dict in the conftest.py is appropriate.
If you want to keep the fixtures in conftest.py, then let's make sure we are consistent and using this approach in all tests.
| assert errors[0]["type"].startswith("enum") | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("bad_pct, expected_type", [ |
There was a problem hiding this comment.
Let's use full names, bad_percentage instead of bad_pct. Same applies for other places.
No description provided.