Package onboarding review

[noamross]

General comments

Great package and project!

• I have very little to say about the codebase itself - it's well designed, and clever in its modularity and extensibility. I have more comments about the documentation below, but I realized that several of them pertain to the fact that function documentation is somewhat cookie-cutter from function to function, as you use roxygen2 inheritance rules to re-use parts of the documentation. Some functions could use more specific documentation, which may require breaking out of this templated approach.
• Is there any way to query the database aside from just providing species names or codes? In general I found myself wondering if I could query the database in other ways. For instance, I'd like to get all species that had a certain diet or met certain morphometric criteria. There are a number of different views on the fishbase website that allow one to do this. Of course, this would require a lot more functions, but perhaps it's worth it to build in some more general way to access the SQL database through the API?

  Tests

• Lots of test and excellent coverage! Passes, though I had to bypass needs_api() because the server was returing "mysql_server_up": false.
• ping()could use a more verbose name. (ping_fishbase_server()`?)

  README.md / vingette

• I dislike the use of "Species list" because it is easy to confuse with an R list. May you can use the term "vector" from the start?
• Some of the output in README.md is prepended with a funny # output:. This probably has to do with a custom knitr setup.
• Nice warning for misapplied names!
• Vingette should document the list of all taxa

  Functions

• Maintain naming consistency: speciesnames --> species_names, commonnames --> commonnames
• commonnames returns ? in place of Mandarin Chinese names. This appears to occur at the server level, though not on the fishbase website. Can the server not serve unicode? If so, I would recommend some notice in the documentation of this current limitation, as it may preclude use by users of non-Roman languages.
• Several tables return a reference field (e.g., DietRefNo from 'diet()), but there is no function (or API functionality) to retrieve those references. It would be useful to be able to look these up.

  Code of conduct

• I recommend adding a code of conduct to the package.

  Documentation

• I'm not sure of the target user, but many might encounter FishBase through rfishbase for the first time. The README/vingette/help files could use some links to relevant fishbase documentation.
• Some examples (e.g., speed("Oreochromis niloticus"), also ration(), ) return zero results. Also the species_list() example in the vingette returns zero resuts. Examples should be queries that return results.
• All function documentation should have a link to the relevant field description pages in Fishbase manual for the fields parameter, or at least standard guidance of how to find the relevant fields in the Fishbase manual. I realized that this documentation is incomplete, but at least many of the fields are described in detail. Another useful approach would be a function (or function option) to return the available fields of a table.
  • Similarly, is there a page that lists available languages that could be linked to in the docs for common_to_sci()?
• species() and species_info() do the same thing. The help file is a bit confusing as the title, usage, and examples alternate between using each. I recommend eliminating/deprecating one usage and making the help file consistent. The species_fields convience object also should be documented in here, or its file should be linked to.

[sckott]

w00t! thanks @noamross

[cboettig]

I think all suggestions here are either addressed or delegated to specific issues and milestone'd appropriately, so I think we can close this.

[cboettig]

@diana-ly Can you create an onboarding review SVG for this package? (Review was in this issue here, prior to those all being on the onboarding repo). Thanks!

[karthik]

@cboettig There is nothing to create. Once Noam or someone else open an issue and adds the right tags, the badge will get automatically created.

[cboettig]

@karthik I don't follow... there's no issue to open. The package went through review long ago and the review is posted right here in this issue, since that's how we were doing it at the time.

[karthik]

If there is no issue on the onboarding repo that simply links to this, there is no way for the badge bot to know. The badge bot does not crawl issues of every repo.

So, you could open an issue in onboarding say "Full review at this link" Add 6/Approved and package as tags.

Then you'll have a badge that you can add to your README.

[karthik]

I see that Noam already did that