Closed dieghernan closed 3 years ago
Hi @jlacko! Thanks for your offer. See here the further steps on submission. It would be great if you can have a look on examples and falling gracefully issue (I always struggle with that), and of course any other boring stuff.
Locally I have Windows, if you have access to any another OS I think it could be also useful for checking.
Hi, I will be happy to help. I am on Linux, which kind of complements your Windows machine. I will look into the boring stuff, and prepare a pull request. This looks fun! J.
I have looked at the package, and it seems in a very solid state; I will propose a very slight tuning of the unit tests, very minor. You seem to have it pretty much wrapped up and ready to go as it stands.
As for the graceful failing I have a question: normally I do this via a HEAD request on the API prior to the actual call, but HEAD means a dependency on {httr} which you have so far avoided; such a dependency is not unusual - see the reverse imports https://cran.r-project.org/web/packages/httr/index.html - but still something that I would not introduce lightly (i.e. without asking first)
Thanks for your feedback. Effectively the reason for developing this package is to avoid {httr}/{curl}, since they do not work on my corporate computer (I think this is related with the firewall policy, but I am not an expert).
See here a solution I implemented in another package due to a warning on CRAN about falling gracefully (tryCatch
approach). This is still quite basic as I don’t get the response of the server as if I would use {httr}, but I think it does the job.
BTW, the package was accepted after this change with no further flags
Makes perfect sense; let it rest then...
Just for your information - the approach I have adopted, which is not applicable in your use case as it relies on httr + curl, is based on this thread from RStudio forums: https://community.rstudio.com/t/internet-resources-should-fail-gracefully/49199/12?u=jlacko
Testing the HEAD with a timeout may be somewhat quicker than downloading the entire file, and the failures a little bit more informative, but your approach works fine within the constraints given - and that is what matters.
Thinking of httr some more - it has stop_for_status function, which I found helpful when waiting for the jsons from API to finish downloading; it felt more fancy than the system sleeps that are necessary when using the lite approach.
I have made a pull request with several suggestions; nothing truly serious so feel free to accept or not.
@jlacko see https://github.com/dieghernan/nominatimlite/commit/58169dfebd369f104a28aad8b9c91f115a442508, now tests are skipped offline and queries falls gracefully.
Let me know if there are further improvements and if is not the case I can try CRAN.
Best,
Diego
I have nothing to add; any further improvement would be a step back :)
Fingers crossed for a straightforward submission!
Sent! Hope a swift submission a267fa9947de46ce353a37184e6d40e99e7a71dc
We have an answer
Thanks,
Please put functions which download data in \donttest{}. e.g. geo_address_lookup.Rd
Please fix and resubmit.
Best, Julia Haider
Hm.... not something that I would remember from reading the CRAN policy as obligatory (I thought it was more of a recommendation; they like their logs clean and server load manageable). But on the other hand: if this is the biggest reservation that CRAN maintainers have - you got off lightly :)
@jlacko https://cran.r-project.org/package=nominatimlite
Thank you very much
tada!!!! congratulations!
First release:
usethis::use_cran_comments()
Title:
andDescription:
@returns
and@examples
Authors@R:
includes a copyright holder (role 'cph')Prepare for release:
devtools::build_readme()
urlchecker::url_check()
devtools::check(remote = TRUE, manual = TRUE)
devtools::check_win_devel()
rhub::check_for_cran()
Submit to CRAN:
usethis::use_version('minor')
devtools::submit_cran()
Wait for CRAN...
usethis::use_github_release()
usethis::use_dev_version()