From 8c23fcee065ee6239b1ceb6138b7204a8935defc Mon Sep 17 00:00:00 2001 From: Jack Davison Date: Fri, 2 Aug 2024 22:16:10 +0100 Subject: [PATCH 01/11] feat: add argument to check providers in `addProviderTiles()` --- R/plugin-providers.R | 21 +++++++++++++++++---- man/addProviderTiles.Rd | 17 +++++++++++------ 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/R/plugin-providers.R b/R/plugin-providers.R index c17f6397f..b1b63cad4 100644 --- a/R/plugin-providers.R +++ b/R/plugin-providers.R @@ -19,10 +19,13 @@ leafletProviderDependencies <- function() { #' ) #' @param layerId the layer id to assign #' @param group the name of the group the newly created layers should belong to -#' (for [clearGroup()] and [addLayersControl()] purposes). -#' Human-friendly group names are permitted--they need not be short, -#' identifier-style names. +#' (for [clearGroup()] and [addLayersControl()] purposes). Human-friendly +#' group names are permitted--they need not be short, identifier-style names. #' @param options tile options +#' @param ... Not currently used +#' @param .check Check that the specified `provider` matches the available +#' currently loaded leaflet providers? Defaults to `TRUE`, but can be toggled +#' to `FALSE` for advanced users. #' @return modified map object #' #' @examples @@ -36,8 +39,18 @@ addProviderTiles <- function( provider, layerId = NULL, group = NULL, - options = providerTileOptions() + options = providerTileOptions(), + ..., + .check = TRUE ) { + if (.check) { + loaded_providers <- leaflet.providers::providers_loaded() + provider <- match.arg( + arg = provider, + choices = unlist(use.names = FALSE, loaded_providers$providers), + several.ok = FALSE + ) + } map$dependencies <- c(map$dependencies, leafletProviderDependencies()) invokeMethod(map, getMapData(map), "addProviderTiles", provider, layerId, group, options) diff --git a/man/addProviderTiles.Rd b/man/addProviderTiles.Rd index 2228b7c6d..20d9d1373 100644 --- a/man/addProviderTiles.Rd +++ b/man/addProviderTiles.Rd @@ -10,7 +10,9 @@ addProviderTiles( provider, layerId = NULL, group = NULL, - options = providerTileOptions() + options = providerTileOptions(), + ..., + .check = TRUE ) providerTileOptions( @@ -33,16 +35,19 @@ providerTileOptions( \item{layerId}{the layer id to assign} \item{group}{the name of the group the newly created layers should belong to -(for \code{\link[=clearGroup]{clearGroup()}} and \code{\link[=addLayersControl]{addLayersControl()}} purposes). -Human-friendly group names are permitted--they need not be short, -identifier-style names.} +(for \code{\link[=clearGroup]{clearGroup()}} and \code{\link[=addLayersControl]{addLayersControl()}} purposes). Human-friendly +group names are permitted--they need not be short, identifier-style names.} \item{options}{tile options} +\item{...}{named parameters to add to the options} + +\item{.check}{Check that the specified \code{provider} matches the available +currently loaded leaflet providers? Defaults to \code{TRUE}, but can be toggled +to \code{FALSE} for advanced users.} + \item{errorTileUrl, noWrap, opacity, zIndex, updateWhenIdle, detectRetina}{the tile layer options; see \url{https://web.archive.org/web/20220702182250/https://leafletjs.com/reference-1.3.4.html#tilelayer}} - -\item{...}{named parameters to add to the options} } \value{ modified map object From de389874bc8a1d3db81c58f16652e0d1bd6355eb Mon Sep 17 00:00:00 2001 From: Jack Davison Date: Fri, 2 Aug 2024 22:21:46 +0100 Subject: [PATCH 02/11] test: add test for checking of tile providers --- tests/testthat/test-tiles.R | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 tests/testthat/test-tiles.R diff --git a/tests/testthat/test-tiles.R b/tests/testthat/test-tiles.R new file mode 100644 index 000000000..d7b592ccd --- /dev/null +++ b/tests/testthat/test-tiles.R @@ -0,0 +1,14 @@ + +testthat::test_that("Checking of tile providers works correctly", { + expect_no_error( + leaflet() %>% addProviderTiles("OpenStreetMap") + ) + + expect_no_error( + leaflet() %>% addProviderTiles("FAKETILESET123", .check = FALSE) + ) + + expect_error( + leaflet() %>% addProviderTiles("FAKETILESET123") + ) +}) From fb917b786118c1b4330dc2b8734053904aa97a27 Mon Sep 17 00:00:00 2001 From: Jack Davison Date: Fri, 2 Aug 2024 22:25:29 +0100 Subject: [PATCH 03/11] test: use an item from `providers` in test --- tests/testthat/test-tiles.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-tiles.R b/tests/testthat/test-tiles.R index d7b592ccd..09b5cef89 100644 --- a/tests/testthat/test-tiles.R +++ b/tests/testthat/test-tiles.R @@ -1,7 +1,7 @@ testthat::test_that("Checking of tile providers works correctly", { expect_no_error( - leaflet() %>% addProviderTiles("OpenStreetMap") + leaflet() %>% addProviderTiles(providers[[1]]) ) expect_no_error( From 037af7af615dc9ed283c49e2ac74c7d80bfae511 Mon Sep 17 00:00:00 2001 From: Jack Davison Date: Fri, 2 Aug 2024 22:27:57 +0100 Subject: [PATCH 04/11] docs: refine function docs for addProviderTiles() --- R/plugin-providers.R | 3 +-- man/addProviderTiles.Rd | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/R/plugin-providers.R b/R/plugin-providers.R index b1b63cad4..f4abedb08 100644 --- a/R/plugin-providers.R +++ b/R/plugin-providers.R @@ -22,7 +22,6 @@ leafletProviderDependencies <- function() { #' (for [clearGroup()] and [addLayersControl()] purposes). Human-friendly #' group names are permitted--they need not be short, identifier-style names. #' @param options tile options -#' @param ... Not currently used #' @param .check Check that the specified `provider` matches the available #' currently loaded leaflet providers? Defaults to `TRUE`, but can be toggled #' to `FALSE` for advanced users. @@ -60,7 +59,7 @@ addProviderTiles <- function( #' errorTileUrl,noWrap,opacity,zIndex,updateWhenIdle,detectRetina #' the tile layer options; see #' -#' @param ... named parameters to add to the options +#' @param ... For [providerTileOptions()], named parameters to add to the options. Not used in [addProviderTiles()]. #' @rdname addProviderTiles #' @export providerTileOptions <- function(errorTileUrl = "", noWrap = FALSE, diff --git a/man/addProviderTiles.Rd b/man/addProviderTiles.Rd index 20d9d1373..c2c6ccc84 100644 --- a/man/addProviderTiles.Rd +++ b/man/addProviderTiles.Rd @@ -40,7 +40,7 @@ group names are permitted--they need not be short, identifier-style names.} \item{options}{tile options} -\item{...}{named parameters to add to the options} +\item{...}{For \code{\link[=providerTileOptions]{providerTileOptions()}}, named parameters to add to the options. Not used in \code{\link[=addProviderTiles]{addProviderTiles()}}.} \item{.check}{Check that the specified \code{provider} matches the available currently loaded leaflet providers? Defaults to \code{TRUE}, but can be toggled From 7c6ee158ba064120d55e31e9e9b5e3a3f3028141 Mon Sep 17 00:00:00 2001 From: Jack Davison Date: Fri, 2 Aug 2024 22:30:00 +0100 Subject: [PATCH 05/11] chore: add NEWS item --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.md b/NEWS.md index 73275ee27..3119983ce 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,8 @@ * Color palette improvements. All color palette functions now support all `{viridisLite}` palettes ("magma", "inferno", "plasma", "viridis", "cividis", "rocket", "mako", and "turbo"). +* `addProviderTiles()` will now error if the chosen `provider` does not match any currently loaded provider (by default, those in `providers`). This behaviour can be toggled off by setting the new `.check` argument to `FALSE` (@jack-davison, #929) + # leaflet 2.2.2 * Fixed #893: Correctly call `terra::crs()` when checking the CRS of a `SpatVector` object in `pointData()` or `polygonData()` (thanks @mkoohafkan, #894). From 1d924145a0076c86322046f777d9796fab1626ad Mon Sep 17 00:00:00 2001 From: Jack Davison <45171616+jack-davison@users.noreply.github.com> Date: Sun, 4 Aug 2024 12:31:27 +0100 Subject: [PATCH 06/11] fix: don't use `match.arg()` for checking tiles Avoids partial matching and overlong error messages Co-authored-by: Joe Cheng --- R/plugin-providers.R | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/R/plugin-providers.R b/R/plugin-providers.R index f4abedb08..9bbdb9eba 100644 --- a/R/plugin-providers.R +++ b/R/plugin-providers.R @@ -44,11 +44,9 @@ addProviderTiles <- function( ) { if (.check) { loaded_providers <- leaflet.providers::providers_loaded() - provider <- match.arg( - arg = provider, - choices = unlist(use.names = FALSE, loaded_providers$providers), - several.ok = FALSE - ) + if (!provider %in% names(loaded_providers$providers)) { + stop("Unknown tile provider '", provider, "; either use a known provider or pass `.check = FALSE` to `addProviderTiles()`') + } } map$dependencies <- c(map$dependencies, leafletProviderDependencies()) invokeMethod(map, getMapData(map), "addProviderTiles", From 57ba1ed9cb8f42788ce1a80791b89a987c98bedf Mon Sep 17 00:00:00 2001 From: Jack Davison Date: Sun, 4 Aug 2024 12:32:34 +0100 Subject: [PATCH 07/11] fix: correct position of quotation marks --- R/plugin-providers.R | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/R/plugin-providers.R b/R/plugin-providers.R index 9bbdb9eba..899f6d917 100644 --- a/R/plugin-providers.R +++ b/R/plugin-providers.R @@ -45,7 +45,11 @@ addProviderTiles <- function( if (.check) { loaded_providers <- leaflet.providers::providers_loaded() if (!provider %in% names(loaded_providers$providers)) { - stop("Unknown tile provider '", provider, "; either use a known provider or pass `.check = FALSE` to `addProviderTiles()`') + stop( + "Unknown tile provider '", + provider, + "'; either use a known provider or pass `.check = FALSE` to `addProviderTiles()`" + ) } } map$dependencies <- c(map$dependencies, leafletProviderDependencies()) From d23854abc9c8e2978ff1211a3574d70e5e1e4363 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Wed, 7 Aug 2024 13:15:28 -0400 Subject: [PATCH 08/11] Rename `.check` to `check` and remove `...` --- R/plugin-providers.R | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/R/plugin-providers.R b/R/plugin-providers.R index 899f6d917..d1c7e7242 100644 --- a/R/plugin-providers.R +++ b/R/plugin-providers.R @@ -22,7 +22,7 @@ leafletProviderDependencies <- function() { #' (for [clearGroup()] and [addLayersControl()] purposes). Human-friendly #' group names are permitted--they need not be short, identifier-style names. #' @param options tile options -#' @param .check Check that the specified `provider` matches the available +#' @param check Check that the specified `provider` matches the available #' currently loaded leaflet providers? Defaults to `TRUE`, but can be toggled #' to `FALSE` for advanced users. #' @return modified map object @@ -39,16 +39,15 @@ addProviderTiles <- function( layerId = NULL, group = NULL, options = providerTileOptions(), - ..., - .check = TRUE + check = TRUE ) { - if (.check) { + if (check) { loaded_providers <- leaflet.providers::providers_loaded() if (!provider %in% names(loaded_providers$providers)) { stop( "Unknown tile provider '", provider, - "'; either use a known provider or pass `.check = FALSE` to `addProviderTiles()`" + "'; either use a known provider or pass `check = FALSE` to `addProviderTiles()`" ) } } @@ -61,7 +60,7 @@ addProviderTiles <- function( #' errorTileUrl,noWrap,opacity,zIndex,updateWhenIdle,detectRetina #' the tile layer options; see #' -#' @param ... For [providerTileOptions()], named parameters to add to the options. Not used in [addProviderTiles()]. +#' @param ... named parameters to add to the options #' @rdname addProviderTiles #' @export providerTileOptions <- function(errorTileUrl = "", noWrap = FALSE, From b882630bdabf85b8ddfe89eeadaad52d22872c8c Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Wed, 7 Aug 2024 13:16:13 -0400 Subject: [PATCH 09/11] Fix arg name --- tests/testthat/test-tiles.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-tiles.R b/tests/testthat/test-tiles.R index 09b5cef89..10c0dca90 100644 --- a/tests/testthat/test-tiles.R +++ b/tests/testthat/test-tiles.R @@ -5,7 +5,7 @@ testthat::test_that("Checking of tile providers works correctly", { ) expect_no_error( - leaflet() %>% addProviderTiles("FAKETILESET123", .check = FALSE) + leaflet() %>% addProviderTiles("FAKETILESET123", check = FALSE) ) expect_error( From 8d3d7c583f4790c85842c0def5cbfb37a974a97e Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Wed, 7 Aug 2024 13:23:49 -0400 Subject: [PATCH 10/11] document --- man/addProviderTiles.Rd | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/man/addProviderTiles.Rd b/man/addProviderTiles.Rd index c2c6ccc84..e9e8d5d71 100644 --- a/man/addProviderTiles.Rd +++ b/man/addProviderTiles.Rd @@ -11,8 +11,7 @@ addProviderTiles( layerId = NULL, group = NULL, options = providerTileOptions(), - ..., - .check = TRUE + check = TRUE ) providerTileOptions( @@ -40,14 +39,14 @@ group names are permitted--they need not be short, identifier-style names.} \item{options}{tile options} -\item{...}{For \code{\link[=providerTileOptions]{providerTileOptions()}}, named parameters to add to the options. Not used in \code{\link[=addProviderTiles]{addProviderTiles()}}.} - -\item{.check}{Check that the specified \code{provider} matches the available +\item{check}{Check that the specified \code{provider} matches the available currently loaded leaflet providers? Defaults to \code{TRUE}, but can be toggled to \code{FALSE} for advanced users.} \item{errorTileUrl, noWrap, opacity, zIndex, updateWhenIdle, detectRetina}{the tile layer options; see \url{https://web.archive.org/web/20220702182250/https://leafletjs.com/reference-1.3.4.html#tilelayer}} + +\item{...}{named parameters to add to the options} } \value{ modified map object From 8c23441692440499b801638d95625a89f61bcca5 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Wed, 7 Aug 2024 13:24:40 -0400 Subject: [PATCH 11/11] Update NEWS.md --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 3119983ce..b0663c6d8 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,7 +2,7 @@ * Color palette improvements. All color palette functions now support all `{viridisLite}` palettes ("magma", "inferno", "plasma", "viridis", "cividis", "rocket", "mako", and "turbo"). -* `addProviderTiles()` will now error if the chosen `provider` does not match any currently loaded provider (by default, those in `providers`). This behaviour can be toggled off by setting the new `.check` argument to `FALSE` (@jack-davison, #929) +* `addProviderTiles()` will now error if the chosen `provider` does not match any currently loaded provider (by default, those in `providers`). This behaviour can be toggled off by setting the new `check` argument to `FALSE` (@jack-davison, #929) # leaflet 2.2.2