Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upSubmission: hydroscoper #185
Comments
|
using cover::package_coverage() using local tests (i.e. comment line 20 in file /test/test_get.R) I get:
and with goodpractice::gp():
|
Editor checks:
Editor commentsThanks for your submission, @kvantas! Reviewers: @timtrice shargelfand |
|
@timtrice thanks for accepting to review this package! Your review is due on 2018-02-04. Please find here our reviewing guide and the review template. |
|
Following @maelle 's advice I enriched error handling about all the sub-domains availability. Thank you very much! |
|
@kvantas for info one of the two reviewers had to drop out this time, so the second reviewer will be @sharlagelfand. @sharlagelfand, thanks for accepting to review this package! Your review is due on 2018-02-21. Please find here our reviewing guide and the review template. |
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: Review Comments
library(hydroscoper)
library(plyr)
subdomain <- "kyy"
stations <- get_stations(subdomain)
coords <- ldply(stations$StationID, function(id) {get_coords(subdomain, id)})
timeseries <- ldply(stations$StationID, function(id) {get_timeseries(subdomain, id)})
I thought this had worked earlier but I may have been mistaken. If I leave it uncommented as it was then the tests pass.
── GP hydroscoper ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── It is good practice to
|
|
Many thanks, @timtrice, for your time and effort! I will start working on these issues as soon as possible. |
|
Thank you @timtrice for your review! CONTRIBUTING.md and CONDUCT.md are two separate files that should both exist. @kvantas do you plan a JOSS submission? If so, could you add a paper.md conform to JOSS guidelines? Thanks. |
|
Oops actually sorry we do not recommend any CONTRIBUTING.md yet! |
|
If you solve bugs @kvantas please update this thread so that @sharlagelfand might know which package version to use for her review. |
|
That's OK, I can add CONTRIBUTING.md. @maelle, I already have a paper.md in inst/paper. I think that @timtrice used an older version of my package and not Anyway, I will follow @timtrice comments and update this thread about the package version. Thanks again, and sorry if I am missing something obvious. |
|
Thanks and no worries! |
|
Hello all, this is a list of the updates/changes I have made in response to @timtrice 's review. 1., 2. I replaced that vignette with a new one:
@timtrice, thanks for your review, it really improved my package's structure and testing. Please let me know if I missed something. @maelle and @sharlagelfand please use the package version |
|
Thanks for your work @kvantas ! |
|
@sharlagelfand friendly reminder that your review is due on 2018-02-21 |
|
thanks for the reminder @maelle! working on it and hoping to wrap up this weekend |
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: 5 Review CommentsI really enjoyed reviewing this package and especially from an end-user standpoint -- I hope you don't mind the specificness of some of these comments with that in mind! :) Tests
This second one just might be my system -- I don't think I have LaTeX on this computer.
Looks like the skipped test is expected, or it is expected to return an error, at least -- do you mind clarifying? Examples run unsuccessfullyThe example for READMEThe README is great. It provides good background on the Hydroscope project itself, what is included in it, and limitations (i.e., that the files are in Greek). I liked the list of what Something that did confuse me is the VignettesI think the vignette(s?) could use a gentler introduction to Hydroscope and the I'm not entirely clear if the Function Documentation
Miscellanious thoughts
I have also submitted a PR with a few fixes on documentation/typos -- nothing major, just when I saw things that could quickly be fixed and didn't need to be brought up here! |
|
Many thanks for your thorough review @sharlagelfand! |
|
@sharlagelfand, many thanks for your review, your time spent and your good words! Thanks a lot, also, for your contribution on the package with the PR. I 'll start working on your comments as soon as possible. |
|
Hello again! I have made the suggested changes: response to @sharlagelfandTests
Examples run unsuccessfullyI exported properly get_instruments_type() in ropensci/hydroscoper@af0297e READMEI added further explanation about Vignettes
Function Documentation
other recommendations
|
|
Thanks @kvantas! @timtrice and @sharlagelfand could you please have a look and say whether you're satisfied with the changes made? |
|
Thank you @sharlagelfand! |
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: Review Comments
Good job, @kvantas! |
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: 6 Thank you for all the changes made @kvantas! I appreciate that you itemized them and linked to the relevant commits -- made for super easy follow up. I think the additional vignette is great (especially for discovery outside GitHub) and the explanations of the All of the comments I had have been addressed, so this package is good to go by me! I also don't know much about JOSS but from poking around, seemed like a good fit. Happy to default to @maelle on this! |
|
Approved! Thanks a lot @kvantas @sharlagelfand @timtrice for your work! I have four suggestions:
Now here is the list of things you have to do before I close this issue
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. |
|
Thanks a lot @maelle, @timtrice and @sharlagelfan. @maelle, on the following days I will apply your suggestions and do what is needed to transfer the repo. Also, I will write a blog post about the package. @timtrice do you mind if I add you as a reviewer? |
|
Not at all; thank you |
We ask that you submit your draft post via pull request a week before the planned publication date so we can give you some feedback. I have a slot open on Tues April 3rd for a draft to be submitted March 27. How does that sound? If you submit earlier we can publish earlier if another is postponed |
|
That would be lovely, thanks @kvantas! |
|
@stefaniebutland, I think that the date is OK. |
|
@kvantas could you please soon transfer your repo? |
|
@maelle, sorry for the delay, some work kept me back. I followed all your suggestions in But I have some problems: a) ropensci/hydroscoper in travis is locked |
|
a) yeah you do not have access, what do you need to do? I'll be traveling soon so please ping one of the editors in the # onboarding Slack channel |
|
d) Appveyor now activated |
|
@maelle, a) I thought that the repo in travis was off Thank you very much again |
|
I submitted the Everything is ok |
|
Fantastic, congrats! |
|
@kvantas Are you still ok with submitting your draft blog post by Tues Mar 27? |
|
@stefaniebutland, I have some small projects to finish, but I think I will be ready for the draft post. |
Summary
What does this package do? (explain in 50 words or less):
hydroscoper is an interface to the Greek National Data Bank for Hydrological and Meteorological Information, Hydroscope. It provides functions to: a) Transform the available tables and data sets into tidy data frames. b) Transliterate the Greek Unicode text to Latin. c) Translate various Greek terms to English.
Paste the full DESCRIPTION file inside a code block below:
URL for the package (the development repository, not a stylized html page):
https://github.com/kvantas/hydroscoper
Please indicate which category or categories from our package fit policies this package falls under *and why(? (e.g., data retrieval, reproducibility. If you are unsure, we suggest you make a pre-submission inquiry.):
data retrieval, because the package parses and download data from a national data-bank.
Who is the target audience and what are scientific applications of this package?
a) Engineers for the development of water resources and environmental studies in Greece, b) scientists that work with hydrological and meteorological data from Hydroscope and c) Greek organizations to submit data and reports to the European Union for the Implementation of the Water Framework Directive.
Are there other R packages that accomplish the same thing? If so, how does
yours differ or meet our criteria for best-in-category?
No other package exists.
If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.
Requirements
Confirm each of the following by checking the box. This package:
Publication options
paper.mdmatching JOSS's requirements with a high-level description in the package root or ininst/.Detail
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: