Closed maelle closed 8 years ago
Thanks for your submission @masalmon Seeking reviewers now
Reviewers: @hrbrmstr Due date: 2016-05-30
@hrbrmstr Due date: 2016-05-30 - hey there, it's been 21 days, please get your review in soon, thanks :smiley_cat: (ropensci-bot)
@hrbrmstr Due date: 2016-05-30 - hey there, it's been 26 days, please get your review in soon, thanks :smiley_cat: (ropensci-bot)
@hrbrmstr Due date: 2016-05-30 - hey there, it's been 31 days, please get your review in soon, thanks :smiley_cat: (ropensci-bot)
on it now (apologies)
thx
geoparser
completely meets the rOpenSci requirements.geoparser
completely meets the rOpenSci requirements.README.md
is created from README.Rmd
geoparser
completely meets the rOpenSci requirements.#' @noRd
is missing from internal functionsNEWS.md
is missinggeoparser
completely meets the rOpenSci requirements.geoparser
makes good use of tests for various API componentstestthat::skip_on_cran()
is mising from the tests that make API callsgeoparser
completely meets the rOpenSci requirements.geoparser
completely meets the rOpenSci requirements (uses appveyor and travis)geoparser
completely meets the rOpenSci requirements.Depends:
should really be in Imports:
Description:
field, 'This packages' will (depending on the day) get a nastygram from CRAN overlords. A suggested altrnative wording is A wrapper for the the Geoparser.io API, which is a web service that identifies places mentioned in text, disambiguates those places, and returns detailed data about the places found in the text. Basic, limited API access is free with paid plans to accomodate larger workloads.
geoparser
completely meets the rOpenSci requirements.geoparser
completely meets the rOpenSci requirements.Undefined global functions or variables: start
caused by NSE dplyr
calls. It's explainable to CRAN but you may want to consider switching to standard evaluation or putting in a start <- NULL
to avoid the CRAN check complaint.Non-standard file/directory found at top level: ‘README.Rmd’
caused by a lack of ^README\.Rmd$
in .Rbuildignore
.geoparser_url()
vs hardcoding it in every function call that does URL access.httr
for API calls, however…purrr::safely()
or tryCatch()
usage which would help to trap errors in the event of a geoparser.io service disruption or local lack of an internet connction. This is prbly not critical for 0.1.0 release but could be a nice addition for a 0.2.0 release.status_code()
than test the individual $
elements.function_df()
and geoparser_parse()
but she would also prbly not mind a short description of what's going on in the transformation, especially if the API changes substantially and it's been a while since she's seen the code. :-)tbl_df()
are going to have namespace collisions, it is usually better to prefix them with the package name; i.e. tibble::tbl_df()
or dplyr::tbl_df()
vs leave them bare. This was done for httr::content()
, dplyr::bind_rows()
& jsonlite::fromJSON()
but not for other function calls.Issue a geoparser query
vs Geoparser query
(not super important)\url{}
or \href{}{}
. This is done in some places but not others.\code{}
is required vs markdown back-ticks (at least for now, there are updates coming to roxygen2 that will enable backticks for marking monosapaced font segments). This happens quite a bit (to me, at least) when I'm copying from vignettes or Rmd files to roxygenValue
section of geoparser_q()
the column names of the data.frame
element item lists should be in monospace (like the ones you get for free with roxygen when using @param
). Also, where there are qualifiers such as see GeoNames.org for a complete list. Subject to change
, it might be useful to wrap those in \emph{}.#' @references \href{https://geoparser.io/docs.html}{Geoparser.io documentation}
to the geoparser_q()
documentation.opencage
package could be converted to an \href{}{} vs \url{}. 8,000
vs 8,192
. It might be useful to clarify this (though I suspect there won't be many users who would hit this condition and have amiguity).vapply
or one of the purrr
classed alternatives over lapply
is recommended for more guaranteed type-safety. Without actually tracing the execution of those functions (see "future self" bullet) it may not buy you much in terms of safety (i.e. if most of those just return list
s). A cursory iteration thorough both functions (once) suggests that this could be a 0.2.0 internal feature.URLencode
should be utils::URLencode
and the associated @importFrom utils URLencode
added.httr::POST()
call in geoparser_get()
you can use httr::accept_json()
vs add the header manually. I see why you manually did the encoding for the body content, though (that was a good addition and I suspect was super-annoying to figure out; again, really nice job!)=
are missing in a few spots (and there are spaces around =
in the vast majority of uses of them, hence the exceptions being noticeable.prettify
is not used but was imported. This was most likely used during development (I've done that as well).$
but in others mutate_()
is used. This really isn't an issue, per se, but consistency might be better in the long run.geoparser_q()
to take a character vector for text_input
vs just a single string@examples
item and/or in the vignette, especially since it's a longer bit of text.Review time: ~2.5 hours (I forgot to clock myself earlier today)
Thank you so much, @hrbrmstr! I look forward to making all the suggested changes! :smile_cat:
Thanks for the thorough review @hrbrmstr.
I don't think you owed me the PR, @hrbrmstr so many thanks! It really was useful! I have learnt a lot. :+1:
Reg.: "Nice use of the "standard"/modern R way to access APIs requring an API key." At least part of it is my copy-pasting the function @noamross had added to opencage
. :smile:
I'm going to summarize what I did:
rev
and ctb
in the authors list.purrr::safely
... Is what I wrote what you had in mind in your comment?Once you all (@hrbrmstr, @noamross & @sckott ) will be happy with the package, I'll transfer the repo to ropenscilabs but also submit it to CRAN. I had discussed the possible API changes with geoparser.io and I think it's ok to submit this version. I have added the API version number to DESCRIPTION.
Thanks, @masalmon! @hrbrmstr, please let us know if the response addresses everything to your satisfaction.
aye. i also re-looked at the code and it's #spiffy.
On Thu, Jun 16, 2016 at 11:10 AM, Noam Ross notifications@github.com wrote:
Thanks, @masalmon https://github.com/masalmon! @hrbrmstr https://github.com/hrbrmstr, please let us know if the response addresses everything to your satisfaction.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/onboarding/issues/43#issuecomment-226515341, or mute the thread https://github.com/notifications/unsubscribe/AAfHtt5d79VJ4YiREio_79tgbK6gjPN_ks5qMWd8gaJpZM4IZlmP .
Thank you @hrbrmstr.
I have done an editors' check and you are approved! Please go ahead and transfer the repository to ropenscilabs
and add the rOpenSci footer. 👏
Awesome, many thanks to you both, I'll do this ASAP!
Transfer done. Next step will be the CRAN submission!
This package is an interface to the geoparser.io API that identifies places mentioned in text, disambiguates those places, and returns data about the places found in the text.
https://github.com/masalmon/geoparser
The geoparser.io API which is version beta. They said "We expect it to remain largely the same for the immediate future. We'd try to make any changes backwards compatible, and support older versions of the API for an extended period of time to allow users to migrate at their convenience. However, the most likely things to change are:
People interested in Natural Language Processing, or anyone with text where they would like to identify geographical information.
I don't think so. Named Entity Recognition (including placenames) in R is currently offered by the openNLP package that requires Java but then the results are not geolocated.
devtools
install instructionsdevtools::check()
produce any errors or warnings? If so paste them below.