Skip to content
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

JOSS review #42

Closed
amoeba opened this issue Nov 13, 2018 · 6 comments
Closed

JOSS review #42

amoeba opened this issue Nov 13, 2018 · 6 comments

Comments

@amoeba
Copy link

amoeba commented Nov 13, 2018

Hi @romunov and @zkuralt , JOSS encourages reviewers to note and discuss review issues on the issue tracker for the software being reviewed so I've created this issue here.

Review: Accept with minor revisions

This is a really interesting Shiny application and I applaud your attention to detail. I found the installation instructions in the README easy to follow and accurate. The Shiny application itself looks clean, works well, and feels intuitive. Overall, this has been one of my easiest reviews so thank you for taking the time to get things on order.

I found a few issues I'd like you to address before I change my review to an Accept. Please take a look and let me know if you agree or disagree on any of them. I've also put some notes that I don't need you to address for JOSS but think might improve your software a bit.

Review items

  • Version: Does the release version given match the GitHub release (v0.9)?

    No, there are no releases for this software. Please make a release on GitHub.

  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?

    I didn't see any automated tests, but I think your instructions in the REAMDE are sufficient to check this item off (so I did). You might consider adding some unit tests for the R code or even some headless tests for the Shiny app at some later point.

  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

    I didn't see these in the README. Please add these sections.

  • Paper

    The reference for split-apply-combine paper needs its DOI of https://doi.org/10.18637/jss.v040.i01. Please add it and re-generated the paper proof.

Non-review notes:

You don't need to address these for me to sign off on the review

  • In README, I see the text "Luckily WGS (EPSG: 4326)". I think this should read, "Luckily WGS 84 (EPSG: 4326) to be more accurate.
  • While the installation instructions listed the packages I need to get the application running, I wonder if you could also put the dependencies in the DESCRIPTION file to make the dependencies more explicit. This would (1) make it possible to install all of the dependencies with devtools::install_deps() and (2) would allow external tools to track the reverse dependency.
@romunov
Copy link
Owner

romunov commented Nov 13, 2018

Thank you @amoeba for your thorough review. Not only do you raise an issue but also offer a suggestion on how to solve it. In my opinion, this is very important in a review process and many could learn from you.

I think we can address all of the valid points you raised. You will hear from us sooner than later.

@zkuralt
Copy link
Collaborator

zkuralt commented Nov 16, 2018

I created a new branch "joss_review" for revised version. @amoeba we will let you know once we check all the review comments.

Cheers!

@romunov
Copy link
Owner

romunov commented Nov 25, 2018

@amoeba I have looked into automatic testing of shiny apps. A package called shinytest does exactly that, however due to some bugs in its code, it does not work properly with applications implemented through shinydashboard package yet. This means that tests would have to be coded manually and some restructuring of the code would be necessary.

Right now, the README/"vignette" describes the steps of importing accompanying data and offers screenshots (basically what shinytest does). Would this satisfy your criterion you describe in point 2 of your review?

@amoeba
Copy link
Author

amoeba commented Nov 28, 2018

Would this satisfy your criterion you describe in point 2 of your review?

Yes. @romunov that'd be fine.

@romunov
Copy link
Owner

romunov commented Dec 1, 2018

@amoeba I believe we've addressed all the issues you've outlined in your review. Let us know what you think.

@amoeba
Copy link
Author

amoeba commented Dec 3, 2018

Hi @romunov, thanks for taking the time to address the issues I've outlined. I took another look and everything looks in order. I'm going to close this and leave a comment for @trallard on openjournals/joss-reviews#1064. Thanks!

@amoeba amoeba closed this as completed Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants