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

fix memoise #154

Merged
merged 2 commits into from
Oct 30, 2021
Merged

fix memoise #154

merged 2 commits into from
Oct 30, 2021

Conversation

dpprdan
Copy link
Contributor

@dpprdan dpprdan commented Oct 28, 2021

{tidygeocoder}'s current CRAN version does not work well with {memoise}, because

you can’t have a default argument with a default value that is the same as the argument name[, no matter if it is the default or not]

(Omissions and addition mine, see below).

I.e. geo(…, lat = lat) is a problem.

library(tidygeocoder)
mgeo <- memoise::memoise(geo)
mgeo("1600 Pennsylvania Ave NW, Washington, DC")
#> Error in FUN(X[[i]], ...): promise already under evaluation: recursive default argument reference or earlier problems?

This can be fixed (mostly), if we change the defaults for lat and long to strings. With this PR we get:

library(tidygeocoder)
mgeo <- memoise::memoise(geo)
mgeo("1600 Pennsylvania Ave NW, Washington, DC")
#> Passing 1 address to the Nominatim single address geocoder
#> Query completed in: 1.3 seconds
#> # A tibble: 1 x 3
#>   address                                    lat  long
#>   <chr>                                    <dbl> <dbl>
#> 1 1600 Pennsylvania Ave NW, Washington, DC  38.9 -77.0

Unfortunately, the value may never match the argument name, default or not.

mgeocode <- memoise::memoise(geocode)
mgeocode(
  .tbl = data.frame(address = "1600 Pennsylvania Ave NW, Washington, DC"), 
  address = address
)
#> Error in FUN(X[[i]], ...): object 'address' not found

But this can be fixed by the user by passing a quoted column name.

mgeocode(
  .tbl = data.frame(address = "1600 Pennsylvania Ave NW, Washington, DC"), 
  address = 'address'
)
#> Passing 1 address to the Nominatim single address geocoder
#> Query completed in: 1 seconds
#> # A tibble: 1 x 3
#>   address                                    lat  long
#>   <chr>                                    <dbl> <dbl>
#> 1 1600 Pennsylvania Ave NW, Washington, DC  38.9 -77.0

Maybe we should add a note about this somewhere? Or is this too niche?

A second commit changes the styling of the function arguments in the source, making them (arguably) easier to find. But I can revert, if you don't agree.

(Also, TIL roxygen always uses double quotes)

@jessecambon jessecambon changed the base branch from main to prep-v1.0.5 October 30, 2021 14:45
@jessecambon jessecambon merged commit feb86d4 into jessecambon:prep-v1.0.5 Oct 30, 2021
@jessecambon
Copy link
Owner

@dpprdan thanks, I've pulled this in and it will be part of the v1.0.5 release. We could possibly put a note on this at the end of the vignette.

@jessecambon
Copy link
Owner

This is the note I've added:

For use with the memoise package, you may need to use character values when specifying input data columns in the geocode() and reverse_geocode() functions. See #154 for details.

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.

2 participants