You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Hi,
One more checklist issue from the JOSS review checklist:
Automated tests: Are there automated tests or manual steps described so that
the functionality of the software can be verified?
I see you have the testthat framework started on the main branch. Are you planning on incorporating unit tests here? One of my major comments when using runSWATpar() are the lack of helpful error messages when arguments are incorrect. For example, my working folder had not been created yet, but instead of an error saying the folder did not exist I received: Error in if (substr(temp, 1, 9) == "TxtInOut_") { : missing value where TRUE/FALSE needed
So, I think including some functions that check file paths are valid and that arguments are the expected types (data.frame, character, vector, etc.) would be an improvement for troubleshooting. These would be useful automated tests that ensure graceful failures and diagnosable errors are returned.
A second point, related to #47 is that there are a lot of functions hidden within server.R or the inst folder. It would be useful to move those to /R where the function could be documented, exported, and tested. I'm not a Shiny developer and don't have a great perspective of best practices for packaging up Shiny apps, so hopefully this is not bad advice! That would also eliminate a check note I get when building the package locally:
* checking dependencies in R code ...
NOTE
Namespaces in Imports field not imported from:
'DT' 'excelR' 'ggplot2' 'htmltools' 'plotly' 'shinyFiles'
'shinydashboard' 'shinyjs' 'spsComps' 'tippy'
All declared Imports should be used.
Refactoring the Shiny app would potentially be a lot of work. But I think at least including some tests for runSWATpar() would be very helpful.
Hi,
One more checklist issue from the JOSS review checklist:
the functionality of the software can be verified?
I see you have the
testthat
framework started on the main branch. Are you planning on incorporating unit tests here? One of my major comments when usingrunSWATpar()
are the lack of helpful error messages when arguments are incorrect. For example, my working folder had not been created yet, but instead of an error saying the folder did not exist I received:Error in if (substr(temp, 1, 9) == "TxtInOut_") { : missing value where TRUE/FALSE needed
So, I think including some functions that check file paths are valid and that arguments are the expected types (data.frame, character, vector, etc.) would be an improvement for troubleshooting. These would be useful automated tests that ensure graceful failures and diagnosable errors are returned.
A second point, related to #47 is that there are a lot of functions hidden within
server.R
or theinst
folder. It would be useful to move those to/R
where the function could be documented, exported, and tested. I'm not a Shiny developer and don't have a great perspective of best practices for packaging up Shiny apps, so hopefully this is not bad advice! That would also eliminate a check note I get when building the package locally:Refactoring the Shiny app would potentially be a lot of work. But I think at least including some tests for
runSWATpar()
would be very helpful.Edit: Cross reference JOSS review thread:
openjournals/joss-reviews#6797
The text was updated successfully, but these errors were encountered: