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

Updated to the latest version of leaflet-providers-parsed.json #23

Merged

Conversation

pushing-boulders
Copy link

Hello. This PR includes the current leaflet-providers-parsed.json file from the xyzservices repository.

The present one is quite stale and doesn't cover the migration of Stamen's tiles to Stadia Maps among other changes.

This also addresses #22

@visr
Copy link
Member

visr commented Mar 26, 2024

Thanks! It looks like Stamen has to be removed from the tests:

Since your link is for the main branch and will therefore change soon, here is also a permalink (press y) for the current commit, in case we want to compare later on: https://github.com/geopandas/xyzservices/blob/e8198dac0c10435c617dff3720aea3ec43561de4/provider_sources/leaflet-providers-parsed.json

@pushing-boulders
Copy link
Author

@visr No problem. I am still somewhat green on all of this, so I apologize. I just added a commit to remove Stamen from the runtests.jl file.

@rafaqz
Copy link
Member

rafaqz commented Mar 28, 2024

You will need to add a dummy apikey="some_apikey" here like:

MapTiler(; apikey="some_apikey")

It looks like they are forcing users to get an api key now

test/runtests.jl Outdated Show resolved Hide resolved
@pushing-boulders
Copy link
Author

Thank you @rafaqz. I see I need to update the test suite for the new MapTiler requirement. Working on that.

@rafaqz
Copy link
Member

rafaqz commented Apr 1, 2024

Oh right its key. You may need to add key to the list here:

https://github.com/JuliaGeo/TileProviders.jl/blob/main/src/provider.jl#L101

We should really make that smarter somehow.

@pushing-boulders
Copy link
Author

pushing-boulders commented Apr 2, 2024

Thanks @rafaqz. I added it there and have the MapTiler ones working, but the NLS ones are failing, even though :apikey is defined in _access(d).

function _access(d)
    for key in (:apikey, :apiKey, :accessToken, :subscriptionKey, :app_code, :key)
        if hasproperty(d, key)
            return key
        end
    end
    return nothing
end

Here's the NLS URL definition:

    "NLS": {
        "osgb63k1885": {
            "url": "https://api.maptiler.com/tiles/{variant}/{z}/{x}/{y}.jpg?key={apikey}"

New to Julia, so I apologize for these errors.

@rafaqz
Copy link
Member

rafaqz commented Apr 3, 2024

No worries at all this code generation from json is kind of extreme magical julia for experienced people as well, including me one year later...

But NLS just doesn't have a keyword in the tests, you will need to add a dummy apikey there too.

Seems like everyone is locking down their API to logged in users only

@pushing-boulders
Copy link
Author

pushing-boulders commented Apr 3, 2024

Thanks @rafaqz. I tried adding this code (I think it is the right one) locally, but the test still fails. I just pushed a commit with this so you can see the issue directly in the testing.

TileProviders.jl run: Error During Test at TileProviders.jl/test/runtests.jl:5
  Got exception outside of a @test
  MethodError: no method matching NLS(; apikey::String)
  
  Closest candidates are:
    NLS(::Symbol) got unsupported keyword argument "apikey"
     @ TileProviders TileProviders.jl/src/provider.jl:169
    NLS() got unsupported keyword argument "apikey"
     @ TileProviders TileProviders.jl/src/provider.jl:169

What's confusing me is that apikey is used by other providers and is defined, unlike key was, in the _access function.

@rafaqz
Copy link
Member

rafaqz commented Apr 3, 2024

Uggh this may be a bug in the JSON as they don't list apikey as a keyword for NLS at all. So our parser doesnt find it and make a keyword option.

We may need to special-case NLS somehow and manually put in the keyword where the others just get it from _access. Sorry thats kind of awful. I'm also happy to comment out the NLS test for now and I'll fix it later.

@pushing-boulders
Copy link
Author

Thanks @rafaqz. Let me check the latest version. Perhaps this was addressed upstream.

🤦 I see what you are talking about now. Sorry for being so dense. I don't have my debugging chops in Julia...yet. Ha!

Since it isn't defined int he JSON definition, the test complains that it isn't defined.

I can comment out the NLS test on my end and recommit after I check upstream. I think that should be addressed there ideally.

@pushing-boulders
Copy link
Author

pushing-boulders commented Apr 3, 2024

Great. There is an open PR for this exact change upstream at the Leaflet Providers source: leaflet-extras/leaflet-providers#547

Found it via the xyzservices issue: geopandas/xyzservices#157

I'll nudge on that thread, too @rafaqz.

@pushing-boulders
Copy link
Author

Interesting. The test passed on my end, but failed here.

Test Summary:        | Pass  Total  Time
TileProviders.jl run |   89     89  3.7s

When I click on details, I just see that the test failed whereas I was seeing details before.

image

@rafaqz
Copy link
Member

rafaqz commented Apr 3, 2024

It was just a github problem. Thanks for pushing this through

@rafaqz rafaqz merged commit 11047a7 into JuliaGeo:main Apr 3, 2024
9 checks passed
@pushing-boulders
Copy link
Author

No problem @rafaqz. Happy to help. Thanks for your guidance.

@martinfleis
Copy link

Hey, found a link over in xyzservices to this. If you're consuming our JSON, it may be worth pinging this upstream issue leaflet-extras/leaflet-providers#457. For you as well as for xyzservices, it would be much easier if the upstream source of tiles was a ready-to-consume JSON.

@rafaqz
Copy link
Member

rafaqz commented Apr 3, 2024

Good point, and thanks for maintaining the json we use! When writing this package its seemed like the only easily parsed option.

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.

4 participants