usgs-r/cyan

cyan's Issues

Technical review...in progress

Following rOpenSci's review template:
https://devguide.ropensci.org/
https://ropensci.org/software-review/
https://github.com/ropensci/software-review/

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

• As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

• A statement of need clearly stating problems the software is designed to solve and its target audience in README
• Installation instructions: for the development version of package and any non-standard dependencies in README

I might change it to:

remotes::install_github("USGS-R/CyAN", build_vignettes = TRUE

The remotes package has fewer dependencies than devtools, devtools is only re-exporting the remotes function anyway, and the "build_vignettes" argument should be set to TRUE so users can easily use the vignette.

• Vignette(s) demonstrating major functionality that runs successfully locally
• Function Documentation: for all exported functions in R help
• Examples for all exported functions in R Help that run successfully locally
• Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Consider using Authors@R in the DESCRIPTION file:
Here's my info for instance:

Authors@R: c(
    person("Laura", "DeCicco", role = c("aut","cre"),
    email = "[email protected]",
    comment=c(ORCID="0000-0002-3915-9487"))

Then maybe add:

BugReports: https://github.com/USGS-R/dataRetrieval/issues

Functionality

• Installation: Installation succeeds as documented.
• Functionality: Any functional claims of the software been confirmed.
• Performance: Any performance claims of the software been confirmed.
• Automated tests: Unit tests cover essential functions of the package
  and a reasonable range of inputs and conditions. All tests pass on the local machine.

---

Review Comments

In the app, it might be nice to have a button to "Load Example Data"...or maybe print out what the user's:

system.file("extdata", "example.db", package = "CyAN")

Once I click a .db file successfully, it would be great to get some feedback from the app (like, right in the "Connect to database" tab. Just a simple "Success!" message maybe.

I like the NHD layer option!!!

Moving on to the Bivariate plot, it would be schnazzy if the drop-downs filtered out the parameters that don't have data. I had to really poke around to find one that worked (since I was just using the example data)

R code review:
I'm generally not a fan of using pipes in package functions. The reason for this is that the user gets less information if there's an error and they are reading the traceback. With pipes, you only know if failed somewhere. With no pipes, you know if failed on the inner_join or whatever.

I'd consider adding a travis check, something like:
https://github.com/USGS-R/EGRET/blob/master/.travis.yml

This makes sure any changes in the future don't break your tests. In that same travis example, travis then sends a check out to "coveralls"...which is a way to check how well your tests cover your code. It's helpful to see these things on each pull request so you know if you've screwed up or not 😊.

See an error when you haven't selected bivariate parameters.

You shouldn't get an error when you haven't selected the parameter. It should just output nothing, and it would be nice if the plots were labeled, too.

NOTICE: upcoming default branch name change

The master branch of this repository will soon be renamed from master to main, as part of a coordinated change across the USGS-R and USGS-VIZLAB organizations. This is part of a broader effort across the git community to use more inclusive language. For instance, git, GitHub, and GitLab have all changed or are in the process of changing their default branch name.

We will make this change early in the week of February 6, 2022. The purpose of this issue is to give notification of the change and provide information on how to make it go smoothly.

When this change is made, forks and clones of the repository will need to be updated as well. Here are instructions for making the necessary changes.

• On GitHub, change the default branch name from master to main (<your repository> -> Settings -> Branches).
• Change the default branch on any forks you have, as well as local clones. (See details below)
• If you have collaborators on this repository, let them know that they will need to change their forked/local repos. Point them to this issue to facilitate the process!
• Search within your repository for "master" so that you can change references (e.g. URLs pointing to specific commit points of files) to point to "main" instead.
• When you are done, feel free to close this issue!

Changing default branches on forks and local clones

First, update your local git configuration so that new repositories created locally will have the correct default branch: git config --global init.defaultBranch main.

Now, for any forks, do the following:

1. Go to <your fork> -> Settings -> Branches and edit the default branch from master to main.
2. Update the settings for your local clone of this fork to match this change.

git branch -m master main
git fetch origin
git branch -u origin/main main
git remote set-head origin -a

How do you connect USGS-R CyAN with the larger CyAN database?

How do I connect the CyAN R package with the larger CyAN database? I read through the vignette and I see how the example uses a small local version that's included. How can I acquire a local version of the larger database? Or is there a way to connect to the database without a local version?