Conversation
|
Hi @dkahle, would you have time to take a look at this PR? happy to make any required changes. Thanks! |
scottmmjackson
left a comment
scottmmjackson
left a comment
There was a problem hiding this comment.
Would you mind writing some unit tests in addition to making some of the changes?
It also look like your https parameter doesn't do anything. It should be inverted to an opt-out because we should be secure by default (I know the rest of the code doesn't do that yet, but it's a good aspiration).
| #' | ||
| #' @param bbox a bounding box in the format c(lowerleftlon, lowerleftlat, | ||
| #' upperrightlon, upperrightlat). | ||
| #' @param zoom a zoom level |
There was a problem hiding this comment.
Could probably use some information on format here.
| #' upperrightlon, upperrightlat). | ||
| #' @param zoom a zoom level | ||
| #' @param maptype light_all, dark_all, light_nolabels, light_only_labels, dark_nolabels, dark_only_labels, | ||
| #' rastertiles/voyager, rastertiles/voyager_nolabels, rastertiles/voyager_only_labels, or rastertiles/voyager_labels_under. |
There was a problem hiding this comment.
small nit: indentation
There was a problem hiding this comment.
This also looks like the line length is different from the rest of the docstring
| #' rastertiles/voyager, rastertiles/voyager_nolabels, rastertiles/voyager_only_labels, or rastertiles/voyager_labels_under. | ||
| #' @param crop crop raw map tiles to specified bounding box. if FALSE, the | ||
| #' resulting map will more than cover the bounding box specified. | ||
| #' @param messaging turn messaging on/off |
There was a problem hiding this comment.
| #' @param messaging turn messaging on/off | |
| #' @param messaging if FALSE, turn messaging off |
Consider also using a more standard parameter name like verbose or silent
| #' @param crop crop raw map tiles to specified bounding box. if FALSE, the | ||
| #' resulting map will more than cover the bounding box specified. | ||
| #' @param messaging turn messaging on/off | ||
| #' @param urlonly return url only |
There was a problem hiding this comment.
specify type, explain that it's the url to the map
| #' resulting map will more than cover the bounding box specified. | ||
| #' @param messaging turn messaging on/off | ||
| #' @param urlonly return url only | ||
| #' @param color color or black-and-white (use force = TRUE if you've already |
There was a problem hiding this comment.
The function signature says this is an enum between color and bw so that should appear in the docstring.
| #' @param https if TRUE, queries an https endpoint so that web traffic between | ||
| #' you and the tile server is ecrypted using SSL. |
There was a problem hiding this comment.
This should be an opt-out. There's no good reason not to use HTTPS when it's available.
| "rastertiles/voyager_only_labels", | ||
| "rastertiles/voyager_labels_under"), | ||
| crop = TRUE, messaging = FALSE, urlonly = FALSE, color = c("color","bw"), force = FALSE, | ||
| where = tempdir(), https = FALSE, ... |
There was a problem hiding this comment.
HTTPS needs to be enabled by default.
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
|
|
||
|
|
||
|
|
||
| get_carto_tile <- function(maptype, zoom, x, y, color, force = FALSE, messaging = TRUE, where = tempdir(), https = FALSE, url){ |
There was a problem hiding this comment.
HTTPS should be enabled by default
| # the map is only a covering of the bounding box extent the idea is to get | ||
| # the lower left tile and the upper right tile and compute their bounding boxes | ||
| # tiles are referenced by top left of tile, starting at 0,0 | ||
| # see http://wiki.openstreetmap.org/wiki/Slippy_map_tilenames |
|
Hi @scottmmjackson |
|
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. |
2 participants
Adding basemap tiles from Carto DB (cleanup from now deleted pull request)
Before you open your PR
roxygen2::roxygenise(".")?Thanks for contributing!