Open gfairbro opened 2 years ago
Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).Estimated hours spent reviewing: 1,5hour
Estimated hours spent reviewing: 1 hour
Hi team gdhelper!
The package is well documented and well-organized. I attempted to run check()
and everything works well.
I just have some minor comments on the package as follows:
gdpplotterr.R
: there is a part commented out from line 30 to line 37. I just think that the document would look better to delete these unused parts. gdpcleanerr
, each testthat code block uses repetitive data generation code. It will be great if helper data could be used to make the script DRY.Reference
which is about details of each function. The content of the first two functions is long, and the function introduction and demonstration of examples look a little bit messy. Overall, these are all very minor suggestions and I find the package has a clear purpose and the functions are easy to use.
Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).Estimated hours spent reviewing: 1.5 hours
Love the package! I think you've done a great job making it easy to work with data from the open government portal. I only have a few minor ideas for improvement:
In the gdpplotterr()
function, I think it would be really cool if you could also pass in a vector of province names as an argument. This would easily allow me to get a quick glance at the GDP's of the provinces i'm interested in. At the same time It's a bit hard to read the current graph output as there are 11 different lines that show up.
Although there is a brief description in the README file, I think it would be super helpful to have an even more flushed out statement of need. ie even though it may be somewhat obvious.. what problem does your package solve? Why is it needed?
Overall really nice job on the documentation! I found it super easy to install the package and get started right away using the different functions. The only thing I noticed was that maybe the vignette could provide some additional context over and above just how the package functions work. For example, it would be great if it provided some additional information about what kind of data the package works on and where to access the data.
Even though the code is well commented, I think there are some areas where more useful (high level) comments could be given. For example, in gdpimporter.R, there is a code block from line 36-52 that I think could be made more clear with a useful comment at the beginning of the block.
Very small thing, but you could consider renaming your functions according to section 1.3 of the rOpenSci packaging guide. What this boils down to is changing your function name structure from objectverb()
to object_verb()
Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).Estimated hours spent reviewing: 1 hour 30 minutes
read.csv()
, the words can be separated by _
, and the function names need not end with "r" which is a package naming convention.gdpdescriberr()
function.gdpdescriberr()
function allows the user to pick the stats, it calculates all the stats irrespective of what the user wanted and then filters the results. This can be further improved by calculating only the stats that the user wanted.globals.R
.Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).Estimated hours spent reviewing:
Again, great work guys! The package is consistent with your other work and the descriptions of your functions are apt. The use of API to extract open source increase the reach of the package and its users.
Minor suggestions:
gdpdescriberr.R
can be decreased from 111 lines to ~70 lines. The variables used can be named using paste0 function or can be appended in a tibble to make a more concise function.gdpplotterr.R
to decrease the code length.Canada
in gdpplotterr.R
is deliberate, it can be mentioned in the function description to enhance consistency of the code with the documentation. If not, then maybe it can be passed as an argument to make the function more modular.
Submitting Author Name: Gabriel Fairbrother Submitting Author Github Handle: !--author1-->@gfairbro<!--end-author1-- Other Package Authors Github handles: (comma separated, delete if none) @aldojasb, @ramiromejia, @gloriawyy Repository: https://github.com/UBC-MDS/gdphelperR Version submitted: Submission type: Standard
Archive: TBD Version accepted: TBD Language: en
Scope
Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
This package is used to download, extract and format data from the open government portal. It also has a data visualisation element which is not listed above.
Data analysts, statisticians or any interested party looking to use data on historical GDP across Canada.
Not with specific scope for this data.
NA
If you made a pre-submission inquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.
Explain reasons for any
pkgcheck
items which your package is unable to pass.Technical checks
Confirm each of the following by checking the box.
This package:
Publication options
[ ] Do you intend for this package to go on CRAN?
[ ] Do you intend for this package to go on Bioconductor?
[ ] Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:
MEE Options
- [ ] The package is novel and will be of interest to the broad readership of the journal. - [ ] The manuscript describing the package is no longer than 3000 words. - [ ] You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see [MEE's Policy on Publishing Code](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) for your manuscript. We make no guarantee that your manuscript will be within MEE scope.*) - (*Although not required, we strongly recommend having a full manuscript prepared when you submit here.*) - (*Please do not submit your package separately to Methods in Ecology and Evolution*)Code of conduct