-
-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PostcodesioR submission #176
Comments
Editor checks:
Editor commentsThanks for this submission to the geospatial area. While I seek reviewers, please look over some checks from Reviewers: @Robinlovelace, @cvitolo
|
|
@Robinlovelace is reviewer one. I've given you extra time since you're currently busy. Please let me know if you are able to get a review in by Feb 6th. 🙏 |
@cvitolo is reviewer 2. Thanks Claudia! |
Sure will give it a go. |
Package Review@karthik Here is my review. @erzk Nice package and works like a charm, well done!
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 4 Review Comments1. Duplicated column namesIt is good practice to output a table in which column names are unique. If there are duplicates, these columns cannot be easily subsetted by name. A table with duplicated column names is obtained when running the following command (taken from vignette):
In
This way, if you have duplicated column names, you get a self-explanatory error. Error: Columns I would suggest to make all the column names unique. 2. Function Documentation:All the exported functions are documented but it would be extremely useful to have more info on the returned values. 3. bulk_postcode_lookup only works with lists with 2 or more elementsNot sure why 4. The documentation of bulk_reverse_geocoding needs updatingThe help page of bulk_reverse_geocoding needs updating:
5. Logic of nearest_ functions*The documentation of 6. Function postcode_autocompleteThere is a typo in the help page: 'an list' should be corrected to 'a list' 7. Functions not mentioned in vignetteThese functions are not mentioned in the vignette:
8. postcode_queryWhat is the difference between 9. terminated_postcodeDescription section needs updating, as it seems to describe one of the It should also explain what it is meant by 'terminated'. 10. Query limitationMany functions have a query limit of 100, can this lead to misleading results? Please expand and document this limitation in the appropriate help pages. 11. LICENSEThis is just a general question/curiosity: the Postcodesio API has an MIT license, have you thought of applying the same license rather than a GPL3? 12. Note in checkWhen I run devtools::check() on the package I get 1 note: Status: 1 NOTE This note can be removed by adding README.Rmd to your .Rbuildignore file. 13. DESCRIPTION fileI noticed you added httr as a dependency. Please consider using the Import field and import only the functions you need, specifying them in the NAMESPACE file, see example below:
14. Lines longer than 80 charactersWhen I run lintr checks, it shows that the following lines are longer than 80 characters:
Please consider to split each of the above lines into two. 15. Avoid sapply()When I run lintr checks, it shows that you use sapply(). 16. VignetteThe vignette runs fast and smoothly, well done! The content of the vignette is pretty much the same as the README. I would suggest not to duplicate the content because it can be difficult to keep them synched. If you want to keep a vignette, in my opinion, this should be a bit more descriptive than a README file (they often resemble the structure of journal papers). You could maybe expand the introduction to include information about postcodeio (open source, based exclusively on open data, etc.), the target audience of your package and its potential applications. In my opinion, your package has great potential not only as a stand-alone tool, but also as a back-end tool for shiny applications to map and filter geospatial information. It could also be a useful dependency for other packages, to filter data based on postcodes rather than counties/local authorities/coordinates (I have a couple of them, rnrfa and rdefra, that could make good use of PostcodeioR). 17. Function documentationAll the exported functions are documented but it would be extremely useful to have:
|
@cvitolo Thanks for the comments. Very useful. I'll address the issues that you mentioned ASAP. |
@Robinlovelace Just wanted to give you a quick ping and see if you've had time to complete your review. Please let us know if you need more time. @erzk Quick note: I’ll be away from the internet for about a week and will be back online around 02/21. So I will be back to editorial activities shortly. |
No but will get on it. |
Package Review
DocumentationThe package includes all the following forms of documentation:
works fine but the style is non-standard (expect a PR incoming)
r1. UK postcodes are a widely used form of geo-referenced and are seldom
r2. Would creating a package that did this be an option? r3. Regarding style, the package does not adhere to Ropensci's style
r4. Lack of support for common spatial classes for input and output. r5. The output of calls to postcodes.io seems to be overly verbose. I am
r6. At times the suggested input formats are not user friendly. I do not
It would be more useful if the user were given an easy function to r7. httr would be better as an Imports than as a Depends (I'm Overall great package, thanks for the opportunity to review it and I |
@Robinlovelace thanks for the review. I was away for a while so I couldn't work on the package but now I'll get back to it. |
@cvitolo I made changes as you recommended. I answered your comments in more detail:
Thanks again for reviewing the code. I still need to address Robin's comments. |
@Robinlovelace Thanks for the comments, they were very helpful. Statement of need was added. r1. I agree. |
@erzk thanks for addressing all my comments. Great job! Please find below few more suggestions.
Maybe your data.frame needs list-columns?
I know it's a tedious task but a full description of the expected output is needed. If you have multiple functions (e.g. A, B, C) generating the same table, you could add the full description to function A. Function B and C can simply contain a link to the description in function A. Does it make sense? Alternatively you can add links to the original API documentation. But please try to add them systematically to every help page.
OK, it makes sense to keep the R functions as similar to the function calls as possible. Do you mind to add a note on this to the package help pages (postcode_lookup and bulk_postcoe_lookup)? One more thing, at the moment if your > postcode_lookup(c("EC1Y8LX", "RG13DQ"))
Error: length(url) == 1 is not TRUE It would be great to make this error more meaningful. Maybe you could add something like this at the beginnig of postcode_lookup(): if (length(nchar(postcode)) > 1){
stop("This function accepts only one postcode, for multiple requests please use bulk_postcode_lookup().")
} This way, when you run the function with the wrong input, the user gets a clear suggestion to fix the problem: > postcode_lookup(c("EC1Y8LX", "RG13DQ"))
Error in postcode_lookup(c("EC1Y8LX", "RG13DQ")) :
This function accepts one postcode, for multiple postcodes please use bulk_postcode_lookup().
OK, let me know when you fix the JSON vs list bit.
Perfect, let me know when the info is added to the documentation.
There is still an inconsistency to fix in the documentation: the Value section says the function returns a list, while in reality it returns a data.frame (as also stated in the Description section).
OK
I would suggest to add a comment on this in the relevant help pages.
OK
Is there a way to change the default API limit of 100 calls? If so, is there a way to change that default parameter in your package as well?
OK
No problem :)
In my opinion, it is better to 'consciously' import the functions you need because that eliminates the problem of masking functions by mistake.
OK
OK
OK
That's fine. It would be very convenient to have links to the original documentation in the help pages as well. |
Comments post-review:
No suggestions at present, other than to download all the boundaries used by the API and use a spatial package such as sf to extract the boundaries for any point. This could link to my prototype https://github.com/Robinlovelace/ukboundaries/ package which I hope (with some encouragement from @karthik and perhaps support from the ONS) to submit to ROpenSci at some point.
It's not a major issue.
I think this is a major issue. R has some add-on packages that make it intuitive to process, analyse and model spatial data. Premier among these for vector data is sf. If it could take
Like @cvitolo I think list columns or empty columns could save you here. Unlikely to find time to put in a PR to do this with the priority being to complete the https://github.com/Robinlovelace/geocompr book at present.
Great work.
Awesome. |
|
Thanks for the revisions @erzk and for the detailed and thoughtful reviews @cvitolo and @Robinlovelace |
I'm satisfied with the submission and the updates, great to be part of the peer review process. If I think of any further issues would they be best as issues in the issue tracker or as comments here? (Guess: it depends on whether they are directly related to the review process.) Fantastic work @erzk! |
It would be best on the issue tracker of the repo itself (the location will change once accepted, but GitHub will redirect appropriately). |
I'm satisfied with the changes, well done @erzk!
…On Mon, 16 Apr 2018, 22:27 Karthik Ram, ***@***.***> wrote:
If I think of any further issues would they be best as issues in the issue
tracker or as comments here?
It would be best on the issue tracker of the repo itself (the location
will change once accepted, but GitHub will redirect appropriately).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#176 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEhdr12paEOboRb3a4ZKcVa4wF_O2V5qks5tpQzMgaJpZM4RRE9J>
.
|
Congrats @erzk! your package has been approved! 🎉 Thank you for submitting and @cvitolo and @Robinlovelace for thorough and timely reviews. To-dos:
Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. ((https://ropensci.org/blog/). If you are, @stefaniebutland will be in touch about content and timing. Final suggestion. If you found feedback from the reviewers valuable, consider adding them as reviewers in the description of your package (this is entirely up to you and not a requirement). If you decide to, you'll need:
|
@karthik All done. |
Thanks @erzk! That was fast! I'll close this issue since the review is complete but you will hear further from Stefanie on the possibility of writing a blog post. |
@erzk Great to hear you're interested in contributing a post about Here are some technical and editorial guidelines: https://github.com/ropensci/roweb2#contributing-a-blog-post. We ask that you submit a draft via pull request at least one week before the planned publication date. At this point there are several blog posts in the pipeline, so perhaps best for you to suggest a deadline by which you would like to submit a draft. |
Summary
It is an API wrapper for http://postcodes.io/ which is a free geolocation service for the UK data.
https://github.com/erzk/PostcodesioR
Geospatial data, because the package deals with UK geospatial data.
Whoever needs information about UK postcodes, outcodes, or places. This package will be particularly useful for social scientists.
yours differ or meet our criteria for best-in-category?
Not that I'm aware of. This package return a rich geospatial data related to postcodes (but not only).
NA
Requirements
Confirm each of the following by checking the box. This package:
Publication options
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.Detail
Does
R CMD check
(ordevtools::check()
) succeed? Paste and describe any errors or warnings:Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:
If this is a resubmission following rejection, please explain the change in circumstances:
If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:
The text was updated successfully, but these errors were encountered: