rOpenSci review: Kelly

[cboettig]

The main issues I noticed were in the documentation, especially to correct the SPARQL query in the README/vignette so that a blank dataframe is not returned.

Thanks! README example hadn't been updated when we switched the default from 2.2.1 version to the 2.2.0 version of the EML spec (as you already figured out).

Documentation

1. Example needed for as_emld().

added!

1. The example for eml_locate_schema() requires having the EML package installed. Consider adding this package to Suggests in package documentation.

Fixed, this example can run using a file from the emld package instead. Also, I think there's not a good use case to justify exporting this function; so I've made it a non-exported file.

1. I was unable to locate a CONTRIBUTING file.

added.

1. I was unable to locate the maintainer in the DESCRIPTION file.

This is in Authors@R, indicated by the "cre" role

Suggestions for improving package documentation:

1. Typo in package description: "faciliate" should be "facilitate" on line 18 of DESCRIPTION.

thanks, fixed!

Suggestions for improving function documentation:

1. For as_emld, consider providing information about the return value of the function.

done.

1. The Description could be improved for as_json, as_xml, and template documentation.

done

1. For template, consider providing more options for the object argument may be helpful.
2. For template, the Value text refers to usage in vignettes however I was unable to locate that.

I'm kind of on the fence as to whether template is really a useful function. It used to be in the README but I dropped it to focus on the three as_* functions as really the only core functions in the package. Curious for feedback on this. Generally, I think it is trying to fill a role that is almost always better served by a more feature-rich package like EML.

Suggestions for improving README:

1. Typo in SPARQL queries section - I believe "names are act as variables" should be "names act as variables"

thanks, done!

1. Consider indenting as_json("hf205.json") line after pipe in SPARQL queries section for readability done

2. The SPARQL query in the example returns nothing as written. I believe the eml PREFIX should be updated to <eml://ecoinformatics.org/eml-2.2.0/>. yes, done!

3. I was able to run everything in the vignette without separately loading the library maggritr. Is this required? I believe it is needed for the examples that use %>%

Functionality

1. The naming scheme is slightly different than the object_verb format suggested in rOpenSci packaging guidelines, however it is consistent with other conversion-based functions. My only concern is that while the function names are not in conflict, they are similar to non-eml specific conversion functions jsonlite::toJSON and xml2::as_xml_document. The emld::as_xml details clarify that the funciton is specific to EML and emld objects -- perhaps it would be useful to clarify this in the as_json documentation as well?

Yeah, this is a great question. As you note, conversion / coercion methods and file IO (read/write) functions tend to reverse this (using verb_object, like read_xml / write_xml). So I think the as_emld etc pattern makes sense.

Good point about as_xml / as_json being ambiguous, but these are defined as S3 generics (like print and plot) providing methods for the emld type specifically, so this isn't really a collision.

1. Although it is rather intuitive, please add the type of return object to the documentation for the functions as_emld, as_json, and as_xml to keep with rOpenSci packaging guidelines (1.6).

Nice, done now!

1. Consider adding top-level documentation to return from ?emld in keeping with rOpenSci packaging guidelines (1.6).

Good catch! Done, thanks.

1. Are functions in as_eml_document.R, validate_units.R, etc. internal functions that should have #' @noRd as per rOpenSci packaging guidelines (1.6)?

I think this only makes sense if the internal function actually has roxygen documentation notes, to prevent the confusion of the function becoming documented in the package manual, etc. Having an internal function with only a @noRd roxygen tag and no documentation doesn't produce any different behavior than omitting it, and could just be confusing.

• On devtools::check() I encountered the following notes:

❯ checking package dependencies ... NOTE
  Package suggested but not available for checking: ‘spelling’

❯ checking top-level files ... NOTE
  Non-standard file/directory found at top level:
    ‘LICENSE.md’

LICENSED.md now added to .Rbuildignore. using install_github("cboettig/emld", dependencies=TRUE) should install all packages listed in Suggests, including spelling.

• the output of goodpractice:gp() similarly suggested that spelling was not available, however this message disappeared after I installed the spelling package.

yup. I wish goodpractice::gp() would direct users to use devtools::install(dependencies=TRUE) so users know they need the Suggested packages installed as well as the Imported packages in order to run tests etc.