kvasilopoulos / uklr

Client to United Kingdom Land Registry
https://kvasilopoulos.github.io/uklr
GNU General Public License v3.0
7 stars 2 forks source link

calling global object `pc` is an issue #4

Open floswald opened 3 months ago

floswald commented 3 months ago

hi there

just had some fun moments with your package (which works great in general!). i was trying to find prices for postcodes. i have a list of postcodes stored in variable pc. your package does no longer work in that case because this line

https://github.com/kvasilopoulos/uklr/blob/5ac90cbf88b0fdc14e0127d6b1b2f123044d9328/R/avail-ukppd.R#L51

refers to pc. the pc object is loaded lazily upon package startup, itself a fairly dangerous practice:

floswald@PTL11077 ~> R                                                   (base) 

R version 4.2.1 (2022-06-23) -- "Funny-Looking Kid"
Copyright (C) 2022 The R Foundation for Statistical Computing
Platform: aarch64-apple-darwin20 (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

  Natural language support but running in an English locale

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

> library(uklr)
> pc
# A tibble: 1,759,911 × 2
   NUTS3 CODE    
   <chr> <chr>   
 1 UKM73 EH10 7DP
 2 UKM73 EH10 7DX
 3 UKM73 EH10 7DY
 4 UKM73 EH10 7DZ
 5 UKM73 EH10 7EA
 6 UKM73 EH10 7EB
 7 UKM73 EH10 7GZ
 8 UKM73 EH17 8SA
 9 UKM73 EH17 8SB
10 UKM73 EH18 1AA
# ℹ 1,759,901 more rows
# ℹ Use `print(n = ...)` to see more rows
> 

I would suggest to rename your pc object to something less obvious, maybe _postcodes , or simply not to load up on package startup.

thanks!

floswald commented 3 months ago

oh. actually it's worse than that! function does not even work without explicitly loading the package:

floswald@PTL11077 ~> R                                                                                                                            (base) 

R version 4.2.1 (2022-06-23) -- "Funny-Looking Kid"
Copyright (C) 2022 The R Foundation for Statistical Computing
Platform: aarch64-apple-darwin20 (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

  Natural language support but running in an English locale

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

> uklr::ukppd_avail_postcodes()
Error in is.factor(x) : object 'pc' not found
kvasilopoulos commented 3 months ago

Hi thanks for the suggestion, that is actually a good point to rename it

For your second point, you are right. pc is loaded as data (lazily) - I never considered this to be an issue. But I guess I could go around it by invoking it via a function - something like that:

ukppd_avail_postcodes <- function() {
  pc <- uklr_pc()
  pc$NUTS3 <- gsub("[0-9]", "", pc$NUTS3)
  pc[!pc$NUTS3 %in% c("UKM", "UKN"), ]$CODE
}

uklr_pc <- function() {
  uklr::pc
}

I think this could work.

By the way, I haven't touched this package for almost 3 years, you are more than welcome to do a PR if you want!

floswald commented 3 months ago

yeah, so I imagined! let's see if i find a moment!