search

rsbivand / rgrass

Interpreted interface between the GRASS geographical information system and R
https://rsbivand.github.io/rgrass/
26 stars 8 forks source link

Fix bug due to change in terra's SpatExtent #79

Closed adamlilith closed 1 year ago

adamlilith commented 1 year ago

SpatExtent objects no longer contain the @ptr slot. Using as.vector() around a SpatExtent object retrieves the extent coordinates.

rsbivand commented 1 year ago

The slot formerly ptr was renamed to pnt in https://github.com/rspatial/terra/commit/c2b6697aed368f17a639a5ce0e0b3d1224e8b463#diff-94c09e9a0dce65c379c47bd63c4c3fb115f0f3b516d145d1fda96458b979eebc (this is as in released 1.7-46), and to cpp in https://github.com/rspatial/terra/commit/c5b34dde3d1b9e98cc953c63d574289ae2e0a57e#diff-94c09e9a0dce65c379c47bd63c4c3fb115f0f3b516d145d1fda96458b979eebc. I'm a bit unsure how to proceed, since CMD check doesn't fail in nc_basic_spm_grass7. We'd need first to add a test that fails with 1.7-46, and then merge your PR, with further checks with 1.7-46, pre-1.7-46, and terra-master. @adamlilith what was your use case when you hit the problem? Best using nc_basic_spm_grass7.

rhijmans commented 1 year ago

I do not think you need to condition on version. as.vector<SpatExtent> has been part of terra from the first CRAN release.

The breaking code uses the x@ptr$..... idiom. I do not consider that a part of the user interface because I would not be able to make changes to it (e.g., adding an argument) without breaking existing code in other packages. This is why I will regularly change its name. Currently it is x@pnt$... on CRAN and x@cpp$... on github. I warned package maintainers beforehand but this one in rgrass did not come up in the reverse dependency checking.

rsbivand commented 1 year ago

@rhijmans Thanks, I understand. No revdep checks are run on examples when GRASS is absent - all the examples run with GRASS. I'll adapt and apply.

rhijmans commented 1 year ago

Thanks. I should also have searched https://github.com/cran. Doing so today helped me find a few more cases that do not show up in reverse dependency checking.

rsbivand commented 1 year ago

I should have looked more carefully when writing the code originally. Revised version on CRAN now.

adamlilith commented 1 year ago

Thanks @rsbivand -- and thanks for the details on terra, @rhijmans, as I didn't know that about the pointer changing name.