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

Add support for Mapbox service #71

Closed
13 tasks done
dieghernan opened this issue Mar 4, 2021 · 1 comment
Closed
13 tasks done

Add support for Mapbox service #71

dieghernan opened this issue Mar 4, 2021 · 1 comment

Comments

@dieghernan
Copy link
Contributor

Following #62 , I have implemented Mapbox rebased on reverse-geocoding branch.

See the checklist below:

Data

  • data-raw/api_reference.R updated
  • Modify R/data.R

Sandbox

  • Added sandbox/query_debugging/mapbox_test.R
  • Added sandbox/reverse_queries/mapbox_reverse.R

Code

  • R/utils.R - Utils added for Mapbox
  • R/query_factory.R updated
  • R/geo_methods.R updated
  • R/geo.R updated
  • R/reverse_geo.R updated

Testing

Some Notes not related with the PR itself:

#> * checking CRAN incoming feasibility ... NOTE
#> Version contains large components (1.0.2.9000)

And this:

Found the following (possibly) invalid URLs:
  URL: https://osm.org/copyright (moved to https://www.openstreetmap.org/copyright)
    From: README.md
    Status: 200
    Message: OK

I checked and that url is generated by the response of the reverse_geocode on the README (see table on https://github.com/dieghernan/tidygeocoder/blob/master/README.md#usage), so not related with the package.

Misc

  • Update README

There are two additional topics about which I would like to gather your feedback,

Integrate Github Action

Github Actions can run R CMD CHECK on a package repo over different platforms and R versions. I use this on my packages, you can see more on https://github.com/r-lib/actions/blob/master/examples/README.md

I included my version while developing. This Github Action would run regularly once a week and after every commit/PR on branchs master or main,, checks are performed on devel, release, oldrel on Linux, macOS and Windows (as CRAN):
https://github.com/dieghernan/tidygeocoder/actions/runs/620783144

Before removing it I just wanted to check if you would like to integrate this on your repo.

About batch geocoding

This is possible on Mapbox but under the commercial endpoint mapbox.places-permanent

https://docs.mapbox.com/api/search/geocoding/#batch-geocoding

I do not have that kind of access, so I didn't include it. I asked Mapbox Team if there is any way I can get a limited dev account for checking, I would let you know.

What I can do if this is strictly required is to include the capability using a mock answer of the API as example (see here), but I won't be able to check it if I don't get a permanent API key (maybe someone with that kind of key could help, @maxachis¿?).

Regards

@jessecambon
Copy link
Owner

jessecambon commented Mar 7, 2021

@dieghernan I tried to put a comment on one of your commits, but I can't find it now so maybe it didn't go through. I've pulled in your code and everything is working well. Good work on this. I added you as an author in the description file.

I also added an issue for whoever wants to add batch geocoding support for Mapbox (#73). We can just have someone who uses the permanent endpoint add it.

I haven't pulled the code into the master branch just yet so I haven't tried out the Github actions yet, but I think that's a good idea. I'll try it out. 👍

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

No branches or pull requests

2 participants