Check ideas

[gaborcsardi]

DESCRIPTION file

• URL
• BugReports
• No Depends
• No Date
• Valid fields (R CMD check does most of this) R CMD check does it
• No whitespace from DESCRIPTION field names R CMD check does it
• No blank lines from DESCRIPTION R CMD check does it

NAMESPACE file

• No import (only import functions)
• No exportPattern
• Related: all external functions are called via ::. See All external function calls use :: #41.

Code complexity

• Functions are short (number of expressions) See Code complexity and readability #42
• Lines are short (number of expressions) See Code complexity and readability #42
• Total amount of code is reasonable See Code complexity and readability #42
• All internal functions are used See Code complexity and readability #42
• Cyclomatic complexity
• Argument order is consistent between functions See Function argument order is consistent #43

S4

Functions to avoid

• attach
• setwd
• sapply
• library/require

Expressions to avoid

• 1:length(x)

Packages to avoid

• multicore, use parallel instead. See Check: avoid using some packages #44

Documentation

• There is a README or package level docs See Checks: documentation #45
• Manual pages for exported functions See Checks: documentation #45
• Manual pages for internal functions See Checks: documentation #45
• All arguments documented See Checks: documentation #45
• Manual pages have examples See Checks: documentation #45
• NEWS file See Checks: documentation #45
• Return values are documented. R CMD check does not catch this, actually. See Checks: documentation #45

Tests

• Test cases OK See Test cases are OK #46
• Test cases code coverage

Code style

• Conforms to a standard Hard to check in general, so closing now.

R CMD check

• Report all problems

[HenrikBengtsson]

Somewhat related: Wish #16: R CMD check --flavor=<flavor>: customized add-on package validation.

[gaborcsardi]

@HenrikBengtsson Looks great, thanks! goodpractice will definitely have flavours, actually it already has something, I called it "styles", flavour is a better word I think.

[HenrikBengtsson]

BTW, is your check list in top box suggestion it is bad to specify Date: in your DESCRIPTION, cf. "No Date"?

[gaborcsardi]

YEs, it is bad to specify Date, that's what I meant.

[HenrikBengtsson]

Why?

[gaborcsardi]

Goes out of date often because people don't update it, and it is not a required field. Another date field is put in automatically by R CMD build.

But again, this is subjective, and in your flavour you can turn it on and off as you like.

[HenrikBengtsson]

Goes out of date often because people don't update it, and it is not a required field.

Yes, I agree outdated Date: info is useless. Recently(?), R CMD check --as-cran started to report on this:

* checking CRAN incoming feasibility ... NOTE
Maintainer: 'John Doe <johndoe@example.com>'
The Date field is over a month old.
* checking package namespace information ... OK

Another date field is put in automatically by R CMD build.

True, but on the other hand the Packaged: field also change whenever the otherwise identical source is rebuilt.

I was mostly curious on your take on this, so nothing important to me. I like to be able to see when a package was "touched" by the developer (info that should be self contained). The Date: field provides that info if maintained. An alternative, which I actually prefer, is that the NEWS file provides a date with every version entry. This gives further information how old "old" is, because the perception of time varies widely between version numbering formats and developers. Unfortunately, not all packages provide dates in NEWS.

I'm experimenting moving away from Date: in some of my packages and only keeping that info in NEWS. The reason for that is mostly to save me a few keystrokes having to update Date:.

[peterhurford]

I'd really like to see a linter that goes through all the functions in your package and ensures that (1) they are prefixed and that (2) they are declared in your DESCRIPTION file. Also should ensure there are no library or require calls. I don't know if lintr or R CMD CHECK does that already?

[gaborcsardi]

@peterhurford library() and require calls are picked up by R CMD check, so we get those automatically.

What do you mean about functions being prefixed? Can you give an example?

I am also not sure about declaring functions in DESCRIPTION. You mean packages, maybe?

[peterhurford]

@gaborcsardi

library() and require calls are picked up by R CMD check, so we get those automatically.

Ah, awesome. 👍

What do you mean about functions being prefixed? Can you give an example?

Like making sure you call goodpractice::gp instead of gp from within your package. I think this may already be covered by R CMD CHECK.

I am also not sure about declaring functions in DESCRIPTION. You mean packages, maybe?

Haha, yeah, I meant packages.

[gaborcsardi]

Like making sure you call goodpractice::gp instead of gp from within your package. I think this may already be covered by R CMD CHECK.

That is not strictly compulsory if you import the function via NAMESPACE. (Or the whole package, although that is not suggested.) But it is true that explicit calls can be a legitimate requirement, and then we can check for this, yes. I'll add it above.

Haha, yeah, I meant packages.

That is checked by R CMD check, too, I think.

[peterhurford]

Came here to suggest you check for code coverage, but I see you already have that. 👍

Other idea: any plan to integrate with https://github.com/jimhester/lintr?

[gaborcsardi]

Yes, we will call lintr as well.

[drisso]

Bioconductor has the BiocCheck package that performs some useful checks (here: https://github.com/Bioconductor/BiocCheck).
A lot of Bioconductor specific checks not relevant here, but some useful, like check direct slot access for S4 objects, use of <<- and browser().

[gaborcsardi]

@drisso 👍

[eribul]

Not sure I like this advice my self (I don't follow it so far ...) but I recently picked up (through discussion during UseR2016) that the use of x$y is considered bad practice for non interactive use, where x[["y"]] should actually be preffered.

One reason is motivated by the following (as stated by Hadley):

> a <- data.frame(xyz = 1)
> a$x
[1] 1
> a[["x"]]
NULL
> 

[gaborcsardi]

@eribul R CMD check already catches partial names IMO, so we have that already I think. Let me know if we don't catch it.

[gaborcsardi]

Closing this now. I opened separate issues for the remaining ones.