Skip to content
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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

JbannisterScottLogic
Copy link

No description provided.

return True


def build_db_query(tile):
Copy link
Contributor

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

Copy link
Contributor

@eveleighoj eveleighoj left a 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)

router = APIRouter()
logger = logging.getLogger(__name__)

DATABASE = {"user": "", "password": "", "host": "", "port": "5432", "database": ""}
Copy link
Contributor

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



@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):
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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

), "Should raise HTTP 400 for invalid tile parameters"


@pytest.mark.asyncio
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants