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

Added Ordnance Survey providers #140

Merged
merged 13 commits into from
Apr 26, 2023
Merged

Added Ordnance Survey providers #140

merged 13 commits into from
Apr 26, 2023

Conversation

aw-west-defra
Copy link
Contributor

@aw-west-defra aw-west-defra commented Apr 25, 2023

Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Two minor notes left in the code.

I'll just need to remind myself to make a follow-up PR adding a test once I store the token in the repo environment variables.

"url": "https://api.os.uk/maps/raster/v1/zxy/Road_3857/{z}/{x}/{y}.png?key={key}",
"html_attribution": "Contains OS data &copy Crown copyright and database right {year}",
"attribution": "Contains OS data (C) Crown copyright and database right {year}",
"key": "<A valid OS MapsAPI Key. Get a free key here - https://osdatahub.os.uk/>",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"key": "<A valid OS MapsAPI Key. Get a free key here - https://osdatahub.os.uk/>",
"key": "<insert your valid OS MapsAPI Key. Get a free key here - https://osdatahub.os.uk/>",

these placeholders need to start with "<insert your" as we use this string to check if a token is required or not (maybe a bit clumsily but that's how it is done now).

provider_sources/xyzservices-providers.json Outdated Show resolved Hide resolved
"Road_3857": {
"url": "https://api.os.uk/maps/raster/v1/zxy/Road_3857/{z}/{x}/{y}.png?key={key}",
"html_attribution": "Contains OS data &copy Crown copyright and database right {year}",
"attribution": "Contains OS data (C) Crown copyright and database right {year}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now also noticed the {year} placeholder here. We currently don't support placeholders outside of the url parameter, so you will need to spell it out here. The attribution here shall always have the current year? We may need to figure out some autoupdating logic within https://github.com/geopandas/xyzservices/blob/main/provider_sources/_compress_providers.py

I can probably take care of it within the follow-up PR alognside tests.

@martinfleis
Copy link
Member

All good now. Thanks!

@martinfleis martinfleis merged commit b772371 into geopandas:main Apr 26, 2023
4 of 5 checks passed
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.

None yet

2 participants