search

ropensci / software-review

rOpenSci Software Peer Review.
292 stars 104 forks source link

Submission: hydroscoper #185

Closed kvantas closed 6 years ago

kvantas commented 6 years ago

Summary

Requirements

Confirm each of the following by checking the box. This package:

Publication options

Detail

kvantas commented 6 years ago

using cover::package_coverage() using local tests (i.e. comment line 20 in file /test/test_get.R) I get:

hydroscoper Coverage: 98.94%
R/get_data.R: 90.91%
R/enhydris.R: 98.04%
R/defunct.R: 100.00%
R/get_tables.R: 100.00%
R/hydro_coords.R: 100.00%
R/hydro_translate.R: 100.00%
R/translit.R: 100.00%

and with goodpractice::gp():

It is good practice to

  ✖ write unit tests for all functions, and all package code in general. 98% of code
    lines are covered by test cases.

    R/enhydris.R:77:NA
    R/get_data.R:64:NA
maelle commented 6 years ago

Editor checks:

Editor comments

Thanks for your submission, @kvantas! :smile_cat: My only note for you and the reviewers is the error handling of that deh subdomain. I'll erase a few comments to keep the thread clean.

Reviewers: @timtrice shargelfand Due date: 2018-02-04 & 2018-02-21 respectively

maelle commented 6 years ago

@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.

kvantas commented 6 years ago

Following @maelle 's advice I enriched error handling about all the sub-domains availability. Thank you very much!

maelle commented 6 years ago

@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.

timtrice commented 6 years ago

Package Review

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

Documentation

The package includes all the following forms of documentation:

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [ ] A short summary describing the high-level functionality of the software
  • [ ] Authors: A list of authors with their affiliations
  • [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

Final approval (post-review)

Estimated hours spent reviewing:

Review Comments

  1. Failed to parse URL (from vignette):
library(hydroscoper)
library(plyr)
subdomain <- "kyy"
stations <- get_stations(subdomain)
coords  <- ldply(stations$StationID, function(id) {get_coords(subdomain, id)})
Warning messages:
1: Failed to parse url: http://kyy.hydroscope.gr/stations/d/501032/

2: Failed to parse url: http://kyy.hydroscope.gr/stations/d/200425/

3: Failed to parse url: http://kyy.hydroscope.gr/stations/d/10014/
timeseries <- ldply(stations$StationID, function(id) {get_timeseries(subdomain, id)})
Warning messages:
1: Failed to parse url: http://kyy.hydroscope.gr/stations/d/10003/

2: Failed to parse url: http://kyy.hydroscope.gr/stations/d/10004/

3: Failed to parse url: http://kyy.hydroscope.gr/stations/d/10005/

4: Failed to parse url: http://kyy.hydroscope.gr/stations/d/10006/

5: Failed to parse url: http://kyy.hydroscope.gr/stations/d/10014/

  1. Should contribution guidelines be named CONTRIBUTING.md rather than CONDUCT.md? (@maelle )

  2. I am unsure about the JOSS questions with the exception that the package does not have a paper.md file as listed (@maelle)

  3. I would recommend using the @title, @description, @details and @section tags in function documentation (just as @param was used). IMO, this improves legibility. This is not required for approval, though.

  4. Install from GitHub did not install all package documentation (missing several of the get* variables). Perhaps the documentation hasn't been fully updated in the master branch?

  5. The tests seem to fail if the skip("Heavy web usage") line is commented,

Exited with status 15.

I thought this had worked earlier but I may have been mistaken. If I leave it uncommented as it was then the tests pass.

  1. goodpractice::gp() output:

── GP hydroscoper ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

It is good practice to

✖ write unit tests for all functions, and all package code in general. 73% of code lines are covered by test cases.

R/enhydris.R:19:NA
R/enhydris.R:20:NA
R/enhydris.R:26:NA
R/enhydris.R:34:NA
R/enhydris.R:42:NA
... and 47 more lines

✖ fix this R CMD check NOTE: Note: found 3031 marked UTF-8 strings

kvantas commented 6 years ago

Many thanks, @timtrice, for your time and effort! I will start working on these issues as soon as possible.

maelle commented 6 years ago

Thank you @timtrice for your review! 😸 And thanks @kvantas for being so reactive! Feel free to wait for the second review if that's easier for you. 😺

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.

maelle commented 6 years ago

Oops actually sorry we do not recommend any CONTRIBUTING.md yet!

maelle commented 6 years ago

If you solve bugs @kvantas please update this thread so that @sharlagelfand might know which package version to use for her review.

kvantas commented 6 years ago

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 0.2.1, as I write in Summary.

Anyway, I will follow @timtrice comments and update this thread about the package version.

Thanks again, and sorry if I am missing something obvious.

maelle commented 6 years ago

Thanks and no worries!

kvantas commented 6 years ago

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: stations_with_data.Rmd that uses the package's data-sets and does not make heavy web usage as the previous one.

  1. I added CONTRIBUTING.md
  2. I moved paper.md and paper.bib from inst/paper to the root directory .
  3. I added @title, @description, @details and @Section tags in function documentation.
  4. I used @rdname to document multiple functions (almost all the get_... functions).
  5. I tide up the internal, API-related functions, wrote tests again utilizingtestthat::skip_on_cran() and a custom function, skip_if_not_online(); there is no need to comment anything in tests now.
  6. Using Sys.setenv(NOT_CRAN="true") and goodpractice::gp() : 
    ♥ Yee-haw! Classy package! Keep up the awe-inspiring work!

    Sys.setenv(NOT_CRAN="true") and covr::package_coverage() returns:

    hydroscoper Coverage: 100.00%

    Sys.setenv(NOT_CRAN="false") and devtools::check() returns:

    0 errors ✔ | 0 warnings ✔ | 0 notes ✔

@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 0.2.3.

maelle commented 6 years ago

Thanks for your work @kvantas !

maelle commented 6 years ago

@sharlagelfand friendly reminder that your review is due on 2018-02-21 😉

sharlagelfand commented 6 years ago

thanks for the reminder @maelle! working on it and hoping to wrap up this weekend

sharlagelfand commented 6 years ago

Package Review

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

Documentation

The package includes all the following forms of documentation:

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [x] A short summary describing the high-level functionality of the software
  • [x] Authors: A list of authors with their affiliations
  • [x] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [x] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

Final approval (post-review)

Estimated hours spent reviewing: 5

Review Comments

I 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

library(covr)
Sys.setenv(NOT_CRAN="true")
covr::package_coverage()
hydroscoper Coverage: 100.00%
R/defunct.R: 100.00%
R/enhydris_api.R: 100.00%
R/get_data.R: 100.00%
R/get_tables.R: 100.00%
R/hydro_coords.R: 100.00%
R/hydro_translate.R: 100.00%
R/translit.R: 100.00%
R/utils.R: 100.00%
library(goodpractice)
goodpractice::gp()
It is good practice to

✖ fix this R CMD check NOTE: Note: found 3031 marked UTF-8 strings
✖ fix this R CMD check WARNING: LaTeX errors when creating PDF version. This typically indicates
Rd problems. LaTeX errors found: ! LaTeX Error: File `inconsolata.sty' not found. Type X to quit or
<RETURN> to proceed, or enter new name. (Default extension: sty) ! Emergency stop. <read *> l.276 ^^M !
==> Fatal error occurred, no output PDF file produced!

This second one just might be my system -- I don't think I have LaTeX on this computer.

devtools::check()
R CMD check results
0 errors | 0 warnings | 0 notes

devtools::test()
✔ | OK F W S | Context
✔ |  1       | defunct related tests
✔ |  1       | Test enhydris_api helper functions
✔ |  2       | enhydris_url returns expected addresses
✔ |  3     1 | Test that expected tables exist in sub-domains' databases [11.6 s]
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
test_enhydris.R:62: skip: deh returns expected tables
deh.hydroscope.gr is not online
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
✔ |  4       | Test enhydris_get and enhydris_list functions⠋ |  1       | Test enhydris_get and enhydris_list functions [21.9 s]
✔ |  1       | Test enhydris_list function [2.5 s]
✔ |  2       | get_data function tests
✔ | 13       | get_tables family function tests
✔ |  2       | hydro_coords tests [0.1 s]
✔ |  7       | Translitetation and translation functions
⠋ |  1       | utils testsping: cannot resolve xxx.hydroscope.gr: Unknown host
✔ |  2       | utils tests

══ Results ════════════════════════════════
Duration: 36.3 s

OK:       38
Failed:   0
Warnings: 0
Skipped:  1

You are a coding rockstar!

Looks like the skipped test is expected, or it is expected to return an error, at least -- do you mind clarifying?

Examples run unsuccessfully

The example for get_instruments_type() fails. This is because it is not exported properly -- in R/get_tables.R I can see it has #' @export get_owners above it instead of #' @export get_instruments_type -- I assume once the correct function is exported, it will run fine (since the example for get_database runs just fine!).

README

The 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 hydroscoper does with data from Hydroscope, and especially how it addresses the limitations to make the data more usable.

Something that did confuse me is the timeseries data set and reference to it. Perhaps it's that I'm missing the context, but I think further explanation of what this data set is would be helpful in both the README, the data set's documentation, and the vignettes. My impression is that it is a lookup table of all of the available measurements for a given station in a given domain, with units of measurement and the time that those measurements are for (hence the name timeseries) -- I think highlighting more concretely what exactly it is a time series of would be useful.

Vignettes

I think the vignette(s?) could use a gentler introduction to Hydroscope and the hydroscoper package, similar to the README. I'm imagining the case where someone finds this package on CRAN, not on GitHub, and needs more background into what the package has (functions, data sets) than is included in the current vignettes.

I'm not entirely clear if the hydroscope_data vignette is in the wrong place (data-raw/ instead of vignettes/), or if it is meant to be used to create the timeseries data set (hence its location) and is not actually a vignette at all! I think the latter, since you created the stations_with_data.Rmd after @timtrice's review, but just wanted to double check :)

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!

maelle commented 6 years ago

Many thanks for your thorough review @sharlagelfand! 👌

kvantas commented 6 years ago

@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.

kvantas commented 6 years ago

Hello again! I have made the suggested changes:

response to @sharlagelfand

Tests

Examples run unsuccessfully

I exported properly get_instruments_type() in https://github.com/kvantas/hydroscoper/commit/af0297e404b0a5e0b162afc96a0455194e98d9cc

README

I added further explanation about timeseries in README, data set's documentation, and the vignettes in https://github.com/kvantas/hydroscoper/commit/5636c7c84993960213eb28844afc0ae5957eb222

Vignettes

Function Documentation

other recommendations

maelle commented 6 years ago

Thanks @kvantas!

@timtrice and @sharlagelfand could you please have a look and say whether you're satisfied with the changes made?

sharlagelfand commented 6 years ago

Hoping to take a look at these changes in the next couple of days! Thanks @kvantas @maelle :)

maelle commented 6 years ago

Thank you @sharlagelfand!

timtrice commented 6 years ago

Package Review

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

Documentation

The package includes all the following forms of documentation:

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [ ] A short summary describing the high-level functionality of the software
  • [ ] Authors: A list of authors with their affiliations
  • [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

Final approval (post-review)

Estimated hours spent reviewing:

Review Comments

  1. Vignettes look much better! Good job.

  2. goodpractice::gp() results are great. 78% code coverage. Very good!

  3. All other issues or comments appear to have been addressed. Again, I'm ignoring the JOSS section as I feel this is beyond my knowledge. @maelle if you want to provide some input or guidance, I'd take it.

Good job, @kvantas!

sharlagelfand commented 6 years ago

Package Review

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

Documentation

The package includes all the following forms of documentation:

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [x] A short summary describing the high-level functionality of the software
  • [x] Authors: A list of authors with their affiliations
  • [x] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [x] References: with DOIs for all those that have one (e.g. papers, datasets, software).

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 stations and timeseries data sets are as well -- really helps to clarify what is contained in the package.

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!

maelle commented 6 years ago

Approved! 👏

Thanks a lot @kvantas @sharlagelfand @timtrice for your work! :clap: :clap: :clap: Note that normally reviewers update the first checklist, but it's fine to have copied it, don't change it now! :smile_cat:

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.

kvantas commented 6 years ago

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? @sharlagelfand is it ok to add you as a contributor/reviewer?

timtrice commented 6 years ago

Not at all; thank you

stefaniebutland commented 6 years ago

I will write a blog post about the package Excellent! Here are some technical and editorial guidelines: https://github.com/ropensci/roweb2#contributing-a-blog-post

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

sharlagelfand commented 6 years ago

That would be lovely, thanks @kvantas!

kvantas commented 6 years ago

@stefaniebutland, I think that the date is OK.

maelle commented 6 years ago

@kvantas could you please soon transfer your repo? 🙏

kvantas commented 6 years ago

@maelle, sorry for the delay, some work kept me back. I followed all your suggestions in https://github.com/ropensci/hydroscoper/issues/11, https://github.com/ropensci/hydroscoper/issues/12, https://github.com/ropensci/hydroscoper/issues/13, https://github.com/ropensci/hydroscoper/issues/14

But I have some problems:

a) ropensci/hydroscoper in travis is locked b) I cannot edit the description in https://github.com/ropensci/hydroscoper c) I cannot find a link in Zenodo

maelle commented 6 years ago

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 b) c) I made you an admin of the repo now, which I couldn't do before transfer 😉

maelle commented 6 years ago

d) Appveyor now activated

kvantas commented 6 years ago

@maelle, a) I thought that the repo in travis was off :cat:

Thank you very much again :tada:

kvantas commented 6 years ago

I submitted the :package: to JOSS, the last thing on the list 😸

Everything is ok 🚀 🚀 ⚡️

maelle commented 6 years ago

Fantastic, congrats! 👏 I'm looking forward to reading your blog post!

stefaniebutland commented 6 years ago

@kvantas Are you still ok with submitting your draft blog post by Tues Mar 27?

kvantas commented 6 years ago

@stefaniebutland, I have some small projects to finish, but I think I will be ready for the draft post. :calendar: