-
Notifications
You must be signed in to change notification settings - Fork 2
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
Tile server migration #262
base: main
Are you sure you want to change the base?
Conversation
application/routers/tiles_.py
Outdated
return True | ||
|
||
|
||
def build_db_query(tile): |
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.
This should all be using sqlalchemy models not raw sql
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.
this needs more work. It needs to imitate how other routers are done in the codebase.
have a look at the search entities function it's not perfect but it's structure should help.
the router function should depend on a database connection, it should then use a data_access function to get the data. the routers job is to then map this data to an output for the user (whether that be a html page or something else)
application/routers/tiles_.py
Outdated
router = APIRouter() | ||
logger = logging.getLogger(__name__) | ||
|
||
DATABASE = {"user": "", "password": "", "host": "", "port": "5432", "database": ""} |
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.
all database info should be managed from the db section this is clearly just development code that was user before. the db folder has the relevant stuff for connecting so don't use whatever is below
application/routers/tiles_.py
Outdated
|
||
|
||
@router.get("/-/tiles/{dataset}/{z}/{x}/{y}.vector.{fmt}") | ||
async def read_tiles_from_postgres(dataset: str, z: int, x: int, y: int, fmt: str): |
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.
this is moderately useful as lays out the main router function but should be using different code underneath.
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 question over the async nature of this, I'm not sure we use it elsewhere in the application. I'm not against it but would need to check it works okay.
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.
I'm not sure the router needs to be tiles_ it should probably just be tiles
tests/unit/routers/test_tiles.py
Outdated
), "Should raise HTTP 400 for invalid tile parameters" | ||
|
||
|
||
@pytest.mark.asyncio |
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.
These don't seem like unit tests, maybe more acceptance tests
No description provided.