Deprecate all Spatial object/sp functionality

[stephhazlitt]

Address part of #109.

• adds note(s) re: deprecation of Spatial object functionality to README
• adds warning on package load re: deprecation of Spatial object fxns (smelly?)
• uses .Deprecate in the convert_to_sp() helper fxn in bcmaps::get_layer(), which is triggered with all the named layer + shortcut functions e.g. bc_bound(class = "sp"), bc_neighbours(class = "sp")
• uses .Deprecate in the methods helper fxns (transform_bc_albers.Spatial andfix_geo_problems.Spatial)
• uses .Deprecate in the Spatial helper fxns get_poly_attribute and self_union

The Approach:

• [x] I am not sure about the .onAttach business, it was a shot in the dark 😬
• [x] combine_nr_rd() returns the warning 2x because convert_to_sp() is called on two layers
• [x] I am not sure how to tackle bc_bbox() with .Deprecate, we could use {lifecycle}?
• [x] This approach returns a warning for users for each/most sp functions, but does not alert users via the package documentation. We could add {lifecycle} as a dependency and leverage the lifecycle badges and Roxygen documentation (and the argument-level functionality for the above situation)—we would need to edit the make_shortcuts() fxn and then regenerate the shortcuts.R script if we want to go that route.

[stephhazlitt]

OK, I'll change it up with {lifecycle}. When do we want to make this defunct—3 months?

[ateucher]

That sounds about right to me. We can put this plus the removal of rgeos into the next release so they go together...

[stephhazlitt]

Turns out we get a lot of mileage from just updating the fxn documentation for bc_bound_hres() since most everything else #' @inheritParams bc_bound_hres. I did end up re-creating the shortcuts.R file, as I removed all of the sp examples from the docs (e.g. no more #' my_layer_sp <- airzones(class = 'sp')). This was inspired by the recommendation to use #' @keywords internal to remove any fully deprecated function from the documentation index in pkgdown sites from the {lifecycles} communicate article.

Sorry for the big PR/review, turns our deprecating sp touches a lot of files in {bcmaps} 😧

[stephhazlitt]

Summarizing the desired approach for deprecating support for sp:

– self_union() & get_poly_attribute(): deprecating whole fxns – transform_bc_albers.Spatial & fix_geo_problems.Spatial: deprecating the Spatial method – bc_bbox(): deprecating the argument value class = "sp" – get_layer() which is used for 35 shortcut functions + combine_nr_rd(): deprecating the class argument – 8 one off fxns that get a layer w/o using get_layer() 👇: deprecating the class argument

• remove tests and examples that rely on the presence of rgeos or rgdal

For get_layer(), the best outcome for the user would be to play with the env and caller_env arguments in deprecate_warn() to raise the level of the warning to the calling shortcut function.

[ateucher]

That's a great summary @stephhazlitt, thanks! I'm also deprecating the whole fix_geo_problems() function as for sf objects it's basically just a wrapper around sf::st_make_valid() - it only really had value for Spatial objects.

[ateucher]

Ok - all green and I think this is ready for another review. @stephhazlitt I can't request a review from you (probably because you are the main PR author), but if you can have a look that would be awesome.

[ateucher]

And of course making a reprex found a bug. Just pushed a fix, but here's some example output:

library(bcmaps)
#> Loading required package: sf
#> Linking to GEOS 3.11.0, GDAL 3.5.3, PROJ 9.1.0; sf_use_s2() is TRUE
#> Support for Spatial objects (`sp`) was deprecated in {bcmaps} v1.2.0, and will be removed altogether in the Summer 2023. Please use `sf` objects with {bcmaps}.

# ensure it always warns, for demo purposes
withr::local_options(lifecycle_verbosity = "warning")

az <- airzones()
#> airzones was updated on NULL
az <- airzones("sp")
#> Warning: The `class` argument of `airzones()` is deprecated as of bcmaps 1.2.0.
#> ℹ Please use `sf` objects with {bcmaps}, support for Spatial objects (sp) will
#>   be removed in Summer 2023.
#> Call `lifecycle::last_lifecycle_warnings()` to see where this warning was
#> generated.

x <- get_layer("bc_cities")
x <- get_layer("bc_cities", class = "sp")
#> Warning: The `class` argument of `get_layer()` is deprecated as of bcmaps 1.2.0.
#> ℹ Please use `sf` objects with {bcmaps}, support for Spatial objects (sp) will
#>   be removed in Summer 2023.
#> Call `lifecycle::last_lifecycle_warnings()` to see where this warning was
#> generated.
x <- get_layer("bc_cities", class = "sf")
#> Warning: The `class` argument of `get_layer()` is deprecated as of bcmaps 1.2.0.
#> ℹ Please use `sf` objects with {bcmaps}, support for Spatial objects (sp) will
#>   be removed in Summer 2023.
#> Call `lifecycle::last_lifecycle_warnings()` to see where this warning was
#> generated.

nr_rc <- combine_nr_rd(class = "sp")
#> Warning: The `class` argument of `combine_nr_rd()` is deprecated as of bcmaps 1.2.0.
#> ℹ Please use `sf` objects with {bcmaps}, support for Spatial objects (sp) will
#>   be removed in Summer 2023.
#> Call `lifecycle::last_lifecycle_warnings()` to see where this warning was
#> generated.

^{Created on 2023-04-06 with reprex v2.0.2}

[stephhazlitt]

This is SO close @ateucher, thanks for the changes. A couple of comments:

• I think we have a bit of inconsistency re: versioning plans. The start up message gives the deprecation timeline as Support for Spatial objects (sp) was deprecated in {bcmaps} v1.2.0, and will be removed altogether in the Summer 2023 and the docs say class = sp is no longer supported and as of bcmaps 1.2.0 (planned for Summer 2023) the class argument will be removed. I think I assumed we would do a release with the deprecation changes (which would bump from 1.1 -> 1.2), and then a subsequent release in the summer with the full removal? Either way, we should align the messages.
• #' @keywords internal IIUC this removes the deprecated function from the help index (and pkgdown site if there is one), removing the user user-facing documentation. Users can only access the deprecation note if they are already users of the fxn and type ?deprecate_fxn in the console. I wonder if in our use case, this is a fast/abrupt design and it we should leave the deprecated fxns in the index?
• This could just be a temporary local issue, but browseVignettes("bcmaps") is not working for me on the PR (but it does with the main branch).

[ateucher]

Thanks @stephhazlitt.

• I think we have a bit of inconsistency re: versioning plans. The start up message gives the deprecation timeline as Support for Spatial objects (sp) was deprecated in {bcmaps} v1.2.0, and will be removed altogether in the Summer 2023 and the docs say class = sp is no longer supported and as of bcmaps 1.2.0 (planned for Summer 2023) the class argument will be removed. I think I assumed we would do a release with the deprecation changes (which would bump from 1.1 -> 1.2), and then a subsequent release in the summer with the full removal? Either way, we should align the messages.

Great catch re the messaging - You're right, it's deprecated in 1.2.0 and will be removed in the summer. I'll align the messages.

• #' @keywords internal IIUC this removes the deprecated function from the help index (and pkgdown site if there is one), removing the user user-facing documentation. Users can only access the deprecation note if they are already users of the fxn and type ?deprecate_fxn in the console. I wonder if in our use case, this is a fast/abrupt design and it we should leave the deprecated fxns in the index?

I think I'm ok removing the function from the index... I don't think we want anyone discovering it now and starting to use it if it's going to be removed in a few months.

• This could just be a temporary local issue, but browseVignettes("bcmaps") is not working for me on the PR (but it does with the main branch).

I'll double-check the vignettes.

[ateucher]

Ok @stephhazlitt I think I've addressed your comments

[stephhazlitt]

Mmmmmmmerge 🚀