stackPlantPresence: Code and documentation review

[schafnoir]

HI @gitbarnitt - I started reviewing code and documentation across all the authors (and making changes myself as a result), and I have a few bits of feedback to share so that the stackPlantPresence function will be even more consistent with the other functions in the package when it comes time for CRAN submission.

Code review:

1. There are a number of variables flagged as "global variables with no visible binding": subplotID, year, divDataType, eventID, namedLocation, plotID, boutNumber, taxonID, identificationQualifier, morphospeciesID ,targetTaxaPresent, scientificName, tot, totalSampledArea.
2. Split out the reformatSubpotID function into its own file. This will be consistent with how other authors have approached multiple functions. If you want it to be a helper function that users do not see that could be an option, but it should still be on its own for testing purposes and consistency with other functions. If helper function, use '.functionName' syntax.
3. Input arguments - add the ability to use both a list from neonUtilities and tables - e.g., add 'input1m2', 'input100m2' arguments. Potentially rename from 'divDataList' to 'inputDataList' to be consistent with general guidance and other PHE, root, and HBP functions.
4. There is lots of commented out code focused on updating subplotIDs --> remove?
5. Error handling: Want basic, informative error handling to accommodate bonehead user mistakes (some of this has already been implemented):
   i. User does not supply a list to 'inputDataList' --> done, but need to update name.
   ii. User supplies a list to 'inputDataList' but list does not have anticipated tables.
   iii. User supplies a list to 'inputDataList' with anticipated tables but required columns missing.
   iv. User supplies both inputDataList AND arguments for input1m2 and input100m2.
   v. User supplies only input1m2 and not input100m2.
   vi. User supplies only input100m2 and not input1m2.
   vii. User supplies input1m2 and input100m2 but input objects are not data frames.
   viii. User supplies input1m2 and input100m2 but required columns are missing from one or both tables.
   ix. User supplies inputDataList or input1m2 and input100m2 but required tables have no data (rows).
   x. User supplies input to argument 'totalSampledAreaFilter' that is not an integer or not in the accepted list of values.

Documentation review:

1. @description - Consider integrating something like this "Data inputs are NEON Plant Presence and Percent Cover data (DP1.10058.001) retrieved using the neonUtilities::loadByProduct() function (preferred), data downloaded from the NEON Data Portal, or input data tables with an equivalent structure and representing the same site x month combinations."
2. @details - could be helpful to indicate what happens if both list and tables are provided. Something like this, "Input data may be provided either as a list generated from the eonUtilities::laodByProduct() function or as individual tables. However, if both list and table inputs are provided at the same time the function will error out."
3. @param divDataList - good details, but update name to inputDataList.
4. @param totalSampledAreaFilter - good details.
5. @return - a little sparse. Any new variables of which end user should be aware and would need defining?
6. @examples - very comprehensive.
7. "ChangeLog" text - remove? Seems like this should not be in the released version.

Testing review:

1. Missing, not yet ready for review.

[schafnoir]

@gitbarnitt - a couple of additional comments to address, and some notes where I fixed output to pass devtools::check() tests, and then I think this function is complete!

• line 13: Explanatory text for 'totalSampledAreaFilter' ends with an incomplete sentence.
• line 225-236: Test dataset doesn't contain any records that test whether 'reformatSubplotID()' function produces expected output.
• line 238-249: Global unbound variables issues for "subplotID" when run through devtools::check(). I resolved, now passes checks.
• line 398: Removed notes-to-self that no longer seem applicable: " Need to figure out how to do unique on specific columns or get rid Different people might have measured the 1 and 10m subplots which could result in otherwise duplicate entries, for example. Maybe have to get rid of the stuff like date above?". Is any of this content development still relevant? If so, likely want to complete before the function is considered done?
• line 418: Removed notes-to-self that no longer seem applicable: "# Data10_100_400 <- rbind(data_10m2, data_100m2)"
• lines 429-439: Global unbound variables issues when run through 'devtools::check()'. I updated code to resolve.
• line 449: Variable "tot" in 'filter()' call caused unbound variable issue; updated to ".data$tot", now resolved.

[schafnoir]

Merged PR