Refactoring to address #124 and #127 + clean up Imports #129
Conversation
- docs: build README to add badges + swap remotes for pak
- refactor: add helper functions + rename insane_longitude_warning to invalid_longitude_warning - refactor: copy mapview2leaflet function into helpers
- refactor: update Imports (adding cli, rlang, sp, utils + removing assertthat) + Suggests (adding geojsonio and stars + moving leaflet.extras, leafpop, mapdeck, miniUI, and tmaptools from Imports) - refactor: stop importing whole libraries, add missing prefixes, and move importFrom tags to package documentation - docs: wrap excess line lengths for function descriptions/parameter definitions - docs: add MIT License file + correct license badge in README
- refactor: split leafletControls.R from projection.R + return mapview_init to leafletControls.R
- refactor: simplify logic for createExtent + add imports from raster, sp, and sf + add helper functions
|
@tim-salabim – no rush, of course, but just wanted to make sure you'd seen that this pull request existed. I also wanted to offer that I can always modify the pull request changes if there was some but not all of these changes that you'd like to see added to the package. |
Also check for libraries loaded in select_shiny_crosstalk
Addresses note from check: "Cannot extract version info from the following section titles: Bug Fixes"
Add Eli Pousson to contributors
|
Hi, sorry for the long silence. I'll get to this asap. |
|
No apologies needed! Just let me know if you have any questions or suggestions. |
|
hey are there any updates? thanks for fixing this |
|
@elipousson I am really sorry, but merging other PRs first led me to implement a few things from here manually given the large list of conflicts... Also, give the CRAN deadline tomorrow, I did end up implementing only selected features of this PR. Apologies for the work that did not make it into main. |
|
Totally understand. I think I've learned to keep my PRs a little more focused since I opened this one, thankfully. I'll close this but try to open a new PR if there is anything else from this one that I think could still be beneficial to carry over. |
|
Thanks for understanding! I've learnt not to let PRs dangle for so long (I hope)... |
I started working on this only to address #124 but, I wanted the package to be able to clear devtools::check(), so I ended up addressing a variety of other small issues at the same time. Most of the changes are focused on avoiding whole library imports + moving the
@importFromtags to the function or package documentation. Key changes to imports include:I added cli and rlang to handle error and warning messages and use
rlang::check_installed()for all the packages that are now in Suggests. I also switched the organization of the non-exported mapview functions to match the organization within the mapview repository.Lastly, I addressed #124 by replacing the calls to
dplyr::select_()withdplyr::select()anddplyr::all_of()and incorporated the fix that @warnes used to address #127 in his pending pull request. Hope this is welcome pull request but I'm happy to make changes if needed!