Add cartoDB basetiles - Githubissues

dkahle / ggmap

A package for plotting maps in R with ggplot2

763 stars 231 forks source link

Add cartoDB basetiles #327

Open gregleleu opened 2 years ago

gregleleu commented 2 years ago

Adding basemap tiles from Carto DB (cleanup from now deleted pull request)

Before you open your PR

[x] Did you run R CMD CHECK?
[x] Did you run roxygen2::roxygenise(".")?

Thanks for contributing!

gregleleu commented 1 year ago

Hi @dkahle, would you have time to take a look at this PR? happy to make any required changes. Thanks!

gregleleu commented 1 year ago

Hi @scottmmjackson Thanks for taking a look. Happy to have a go at the changes, but just to be sure: get_carto currently mimics all other get_* functions (google, stamen etc.). The changes you suggest make sense for all of these. Are we sure we want this one to diverge from the others? Regarding the unit tests: there are no tests for the other get_* functions. So not sure what to test for: I could save some tiles and compare results, but what if tiles change on the carto server?

scottmmjackson commented 1 year ago

As far as tests, a simple smoke test that proves no blatant runtime errors would suffice.

For the changes, the only one I really care about is inverting https and we can diverge for that. For the rest, use your judgment, but I would like types documented a little clearer.