search

ropensci / stplanr

Sustainable transport planning with R
https://docs.ropensci.org/stplanr
Other
420 stars 66 forks source link

CRAN submission issues #486

Closed Robinlovelace closed 2 years ago

Robinlovelace commented 2 years ago 
Flavor: r-devel-linux-x86_64-debian-gcc
Check: R code for possible problems, Result: NOTE
   File 'stplanr/R/zzz.R':
     .onLoad calls:
       packageStartupMessage(paste0("Loading stplanr v0.9.0.\n", "Note:
the next planned version, v1.0.0, will not support sp objects.\n",
"See the issue #332 and https://github.com/ropensci/stplanr/pull/481.",
    "Any feedback on GitHub: welcome. Thanks (Robin Lovelace, April
2022)!"))

   See section 'Good practice' in '?.onAttach'.

   Found if() conditions comparing class() to string:
   File 'stplanr/R/google-functions.R': if (class(from) ==
"SpatialPoints" | class(from) == "SpatialPointsDataFrame") ...
   File 'stplanr/R/google-functions.R': if (class(to) == "SpatialPoints"
| class(to) == "SpatialPointsDataFrame") ...
   File 'stplanr/R/oneway.R': if (class(x) == "factor") ...
   File 'stplanr/R/oneway.R': if (class(y) == "factor") ...
   File 'stplanr/R/oneway.R': if (class(x) == "factor") ...
   File 'stplanr/R/oneway.R': if (class(y) == "factor") ...
   File 'stplanr/R/route-transport-api.R': if (class(from) ==
"SpatialPoints" | class(from) == "SpatialPointsDataFrame") ...
   File 'stplanr/R/route-transport-api.R': if (class(to) ==
"SpatialPoints" | class(to) == "SpatialPointsDataFrame") ...
   File 'stplanr/R/route_cyclestreets.R': if (class(from) ==
"SpatialPoints" | class(from) == "SpatialPointsDataFrame") ...
   File 'stplanr/R/route_cyclestreets.R': if (class(to) ==
"SpatialPoints" | class(to) == "SpatialPointsDataFrame") ...
   Use inherits() (or maybe is()) instead.
Robinlovelace commented 2 years ago

I think the .onAttach() message is a non-issue as we already follow best practice here:

Loading a namespace should where possible be silent, with startup messages given by .onAttach. These messages (and any essential ones from .onLoad) should use [packageStartupMessage](https://rstudio.robinlovelace.net/s/d5de8cf239dd2bbe73918/help/library/base/help/packageStartupMessage) so they can be silenced where they would be a distraction.
agila5 commented 2 years ago

Hi Robin! I'm not sure if this is relevant, but I think the packageStartupMessage should be included in .onAttach not .onLoad:

https://github.com/ropensci/stplanr/blob/9d4b416d891ae023b85141809216eb1ed41881d0/R/zzz.R#L1-L10

Robinlovelace commented 2 years ago

Hi Robin! I'm not sure if this is relevant, but I think the packageStartupMessage should be included in .onAttach not .onLoad:

Thanks for the tip but I'm not sure why you think that. From https://community.rstudio.com/t/when-to-use-onload-vs-onattach/21953 it seems that onAttach() seems to trigger more frequently.