Skip to content

Tile server migration - #262

Open
JbannisterScottLogic wants to merge 16 commits into
mainfrom
tile-server-migration
Open

Tile server migration#262
JbannisterScottLogic wants to merge 16 commits into
mainfrom
tile-server-migration

Conversation

@JbannisterScottLogic

Copy link
Copy Markdown

No description provided.

Comment thread application/routers/tiles_.py Outdated
return True


def build_db_query(tile):

Copy link
Copy Markdown
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

@eveleighoj eveleighoj left a comment

Copy link
Copy Markdown
Contributor

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)

Comment thread application/routers/tiles_.py Outdated
router = APIRouter()
logger = logging.getLogger(__name__)

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

Copy link
Copy Markdown
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

Comment thread 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):

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread tests/unit/routers/test_tiles.py Outdated
), "Should raise HTTP 400 for invalid tile parameters"


@pytest.mark.asyncio

Copy link
Copy Markdown
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