-
-
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
submission: eia #342
Comments
Editor checks:
Editor commentsHello @leonawicz, thanks for submitting to rOpenSci. I'll be your handling editor. The following is the output of
Reviewers: @ottlngr and @daranzolin |
Hi @melvidoni Thank you for considering this submission. I have addressed your requested changes, details below, along with a couple questions.
Unit testsThe package has 100% If the tests are run without an API key present, the vast majority of tests must be skipped. To run/build/check locally an EIA API key must be placed somewhere like your R CMD check errors and warningsI am unable to reproduce these on various Windows or Linux platforms. Is there more information about the environment in which you are building or checking the package? Even without a key present, it will skip the impacted functions and tests and so should still successfully build even in that case. Thank you! :) |
Hello @leonawicz, thanks for taking care of this. Don't worry about the remaining
So I'm going to start searching for reviewers! |
Hello @leonawicz. The first reviewer is @daranzolin. I will establish a deadline for the review as soon as I get a second reviewer in. |
Just a heads up to both @leonawicz and @daranzolin. I am having issues finding a second reviewer. Please bear with me while I email more people. |
We got reviewers assigned, @leonawicz! Thanks @ottlngr and @daranzolin for reviewing this package. You have until October 28th to submit the review through the GitHub issue. |
Hi all, review is below. Package Review
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 2
Review CommentsTerrific package that is already mostly polished. I learned how to get my API key, peruse the vignettes, search through various child categories, and subset relevant data in less than 30 minutes. I felt free to explore aggressively knowing about the built-in memoization and rate-limiting. Great features. The pattern of searching for parent/child ids and using Some questions/observation:
Some alerts from ✖ avoid long code lines, it is bad for readability. Also, many people prefer
Looks like most pertain to And these last two are possibly the result of my machine, not yours, but I'll flag them anyway: ✖ fix this R CMD check WARNING: LaTeX errors when creating PDF version. This typically indicates Rd problems. ✖ fix this R CMD check ERROR: Re-running with no redirection of |
Hello @ottlngr! How is your review going? |
Hi all, I plan to mostly get to work on this once both reviews are in but I can chime in on some initial things. @daranzolin Thank you for your review. :) Regarding your questions about exploring the category tree, I will continue to give some thought to this but the API is vast and I'd want to be extra careful with recursion, possibly recursing up ( I like the idea of allowing names like "Electricity" in place of their opaque category ID numbers. However, these numbers are actually better, in part because no one would want to type out many of the category names, which can get really long. Please let me know if you envision something different or I'm misunderstanding. But yes, I agree, the most frustrating part of such a massive collection of datasets behind an API is getting acquainted with the IDs you need for your project. My hunch is most users do this through the API explorer on the website before ever encountering this particular API wrapper package. I will address the other inconsistencies in the code and documentation you mentioned soon. I think I fixed all the code lines that were longer than 80 characters, but is it necessary to apply this rule to all the roxygen2 help file text? It has no impact on how help docs render and from an author/maintainer standpoint it's a lot easier to write, read and edit them if the sentences aren't so broken up. I'm happy to do so if required, just let me know. I agree on the value of visualizations and will be adding some. The console-heavy feel is too much without having any. I'll plan to add some basic visualizations as part of this review process. Yes I think the errors relate to possibly not having the pdf-creating utility installed on a Windows machine if you are checking/building the package locally. Thanks so much for your feedback! |
@melvidoni @leonawicz I'm planning to finish my review on sunday :) |
@leonawicz We recently developed an internal shiny gadget with cascading select filters from user input, and it struck me as a possible application here. Probably superfluous here, though. I agree with you on both points: the API explorer is sufficient and also shows the category ids (obviating the need to type long strings). |
@ottlngr Sounds good, I will check back next week. Thank you @melvidoni @daranzolin I have had a chance today to make revisions. The latest version on GitHub cleans up the vignette formatting. I also added basic example ggplot graphs to the readme, getting started vignette, and the most applicable other vignettes- series and geoset, per your suggestion. Out of general curiosity, can you point me to a resource about this cascading select filter Shiny gadget you mentioned? Also, after merging your vignette PR, I realized I brought in a new feature with this recent push. I don't want that to confuse anyone, but there is a new function Thanks! :) |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 2.5
Review Comments
A few questions/remarks:
Regarding @daranzolin 's review, I agree with using some visualizations in the vignettes. Also, I wished the exploration of the category tree could be improved - but as you mentioned, @leonawicz, this would result in many additional API requests.
Great package, definitely a good supplement to the rOpenSci universe! |
All reviews are in @leonawicz ! |
@melvidoni thank you and @ottlngr thank you for your review, responses below. The tidy argumentHere was my thought process when developing this: I agree that So I stuck with three values for one argument, and these three values do capture the meaning. I also figure that very few users will ever want something other than I would like to keep it to a single 3-valued argument, but I am open to suggestions along those lines. I considered things like httr functionsHonestly I didn't even know about User agentThanks! That was my first time doing that. Just for context, that is one thing that would have to be updated following any repository transfer and it would be up to ROpenSci if the new user agent was the new repo url or something else. I have no opinion on that but it would have to change from what it is now. On that note, the other breaking effect of a transfer would be the encrypted API key in Next steps@melvidoni @daranzolin @ottlngr Okay, I think I have addressed all reviewers' and editor's comments and questions, provided clarifications, etc. I've made updates that you all called for such as the addition of graphs in the README and vignettes, fixes to documentation, etc. There wasn't a lot of explicit "change this, fix that" in the reviews so I'd like to turn it over to you all to review the changes I did make, e.g., are the basic demo graphs acceptable? I did use Is there anything you all firmly want changed that I missed? Should we continue discussion on certain topics? Thank you! |
@leonawicz graphs look good. No further comments or suggestions from me. But as for our previous discussion on cascading selectInputs in Shiny, here's a good example. @melvidoni I updated my original review comment confirming that the package author has responded to my review. |
@leonawicz Thanks for your detailed answers and sharing your reasoning, especially on the No further questions, suggestions or comments.
@melvidoni I updated my review. |
Approved! Thanks @leonawicz for submitting and @ottlngr and @daranzolin for your reviews! To-dos:
Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them Welcome aboard! We'd love to host a blog post about your package - either a short introduction to it with one example or a longer post with some narrative about its development or something you learned, and an example of its use. If you are interested, review the instructions, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions. We've put together an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here. |
Hi all, @ottlngr @daranzolin thanks so much for your help reviewing the package! And thanks @melvidoni for your guidance. I have transferred the repo and made the package changes affected by the transfer. It appears my encrypted API key in |
Hello @leonawicz! I already gave you admin rights on your repo, and invited you to an rOpenSci team. Can I publish this in Twitter? If so, please, let me know your handle. |
Hi @melvidoni I see the team and settings tab now, thanks! Yes, we're all set to move forward. I just pushed a |
Submitting Author: Matthew Leonawicz (@leonawicz)
Repository: https://github.com/leonawicz/eia
Version submitted: 0.3.3
Editor: @melvidoni
Reviewer 1: @daranzolin
Reviewer 2: @ottlngr
Archive: TBD
Version accepted: TBD
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):
The package specifically is for retrieving energy-related datasets from the US Energy Information Administration (EIA) open data API.
Researchers, data analysts, etc. in academia, government and the energy industry and related industries. While it is the US EIA, international energy datasets are available through the API as well.
There are two other overlapping R packages. (1)
EIAdata
, also on CRAN. This package provides basic data access, but data formats and other options are more limited. It does not cover as many API endpoints or offer as much functionality. (2) There is also a differenteia
, github only: https://github.com/krose/eia. It is similar to (1) in its offerings and level of development. Both are fairly minimalist packages that have been around for about five years (around when the EIA API was first launched, I believe) but have not been developed much further since initial releases.This submission for
eia
(CRAN) is for a package with greater coverage in terms of functionality, API endpoint access, documentation, and robust test coverage and error handling, In comparison to the others it also offers three data output format options for users working with the API data in different contexts (for example, strict R usage or wanting pure JSON results to pipe to other software applications). You can control the level of "tidy"-ness. It also includes a user agent in calls and takes other steps to play well with the API.Technical checks
Confirm each of the following by checking the box. This package:
Note added by author: This particular API requires users to use their own API key. I cannot run function examples or unit tests on CRAN, but all examples and unit tests run successfully in multiple other environments, on local and remote systems. Full test suite also runs on Travis-CI where I am able to import an encrypted key. Previously for CRAN purposes, a no-key copy of the main vignette was included (API call examples not executed) in the package, while the main and all additional vignettes showing executed API calls and results are part of the pkgdown web documentation. Also for CRAN purposes, function examples in the R package source code are not run if they require an API key to be present.
Publication options
JOSS Options
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.MEE Options
Code of conduct
The text was updated successfully, but these errors were encountered: