[Rd] RCMD CHECK and non-methods

Wed Sep 1 00:18:23 CEST 2010

Thanks for those (very different!) responses. I was about to respond to Martin's question about doco amendments, when I saw Simon's reply. 

Case 1: If Martin completely prevails, then I guess the appropriate amendment to the documentation would be in R-extensions, section 1.3.1 "Checking packages", item 10:

  <<... In addition, it is checked whether methods have all arguments of the corresponding generic,...>>

would become

  <<... In addition, it is checked whether potential methods[1] have all arguments of the corresponding generic,...>>

where [1] refers to a footnote:

  <<"Potential methods" here includes any function whose name starts with a known S3 generic followed by a period, e.g. 'subset.blahblah.blah'. This applies regardless of whether the function is formally registered as an S3 method (see 1.6.2), and even if the function is not intended to be a method at all. It is bad practice to give method-like names to non-methods.>>

For absolutely strict super-nanny-level consistency, maybe there ought also to be a change somewhere in the QC/codoc code, to detect "methods" that haven't been registered with 'S3method', and/or have non-generic-style USAGE sections, regardless of whether they happen to have arguments compatible with whatever generic R thinks they belong to. At present, there is a difference in what 'tools::checkDocStyle' and 'tools::checkS3methods' regard as a method. However, enforcing this extra check does strike me as a bit draconian, and would certainly break back-compatibility (in other bits of my code, for a start). You'd have to ask whether it's worth the effort. In my example, I don't think it's worth worrying that anyone is ever going to define an S3 class called 'with.warning' and decide to call 'subset' on it.

Case 2: If Simon completely prevails, then there should be changes in 'tools::checkS3methods' to exempt "documented non-methods". I think the appropriate definition of "documented non-methods" would be functions with a USAGE section that includes the full function name rather than the generic. That's supposed to cover packages with and without NAMESPACEs.

WRTO Simon's comment: I feel the same negative way about camel-case for the same reasons, and would want a much stronger reason to change the naming convention I've been using for 15 years; it's hard enough to remember what name I gave to a function, letAlone how_I_decided to.spell ItThatWeek.

However, I really really hope that S3 *doesn't* get replaced by S4! S3 is very easy to use, and for me it "just works" 99% of the time. Just occasionally it doesn't, but for me at least "the [S4] cure is worse than the [S3] disease". I'll keep an open mind about R5 ;)

Mark Bravington
CSIRO CMIS
Marine Lab
Hobart
Australia
________________________________________
From: Simon Urbanek [simon.urbanek at r-project.org]
Sent: 27 August 2010 02:46
To: Martin Maechler
Cc: Bravington, Mark (CMIS, Hobart); r-devel at r-project.org
Subject: Re: [Rd] RCMD CHECK and non-methods

On Aug 26, 2010, at 11:36 AM, Martin Maechler wrote:

>>>>>>  <Mark.Bravington at csiro.au>
>>>>>>    on Wed, 25 Aug 2010 14:06:07 +1000 writes:
>
>> I recently moved a function 'subset.with.warning' into the 'mvbutils' package (a version not yet on CRAN). When I tried RCMD CHECK, I got this warning:
>> * checking S3 generic/method consistency ... WARNING
>> subset:
>> function(x, ...)
>> subset.with.warning:
>> function(x, cond, mess.head, mess.cond, row.info, sub)
>
>> See section 'Generic functions and methods' of the 'Writing R Extensions'
>> manual.
>
>> I know that S3 method arguments need to be compatible with arguments of the generic. However, 'subset.with.warning' is deliberately not a registered S3 method,
>
> I think that's your real trouble ... and then your users' if you
> really insist.
> Short answer:  "Don't do that!"
>
> There have been a few exceptions of "100 years old" R functions
> which validated this rule,
> the most notable probably  t() and t.test(),
> and we (R core) have explicitly listed them in the code base
> as "yes, looks like a method for an S3 generic, but not it ain't!",
> but have basically "forbidden" to commit more such crimes.
> {Also, if you are interested: I think both  t() and t.test()
> pre-dated S3}
>

However, one would hope that S3 will be replaced by S4 (or R5? ;)) eventually so requiring such an arcane rule seems little strong. Personally, I prefer the use of . to that of camelCase [I find it more readable and faster to type] and thus I'm with Mark on this one. IMHO it is perfectly legal and clean to declare a symbol as a function and not an S3 method.

Cheers,
Simon

>
>> and its USAGE section doesn't include a \method{generic}{class} statement. I couldn't see anything in "R Extensions" that says "don't do this", so I'm wondering:
>
> Yes, we should add a such "don't do this" somewhere.
>
> Can you propose a good place to put it in there?
>
>> - should this really be a NOTE not a WARNING (or nothing at all)?
>
> {from the above: definitely a warning, if not "worse"}....
>
> Best regards,
> Martin
>
> Martin Maechler, ETH Zurich
>
>> - if not, shouldn't there be a more explicit statement to the effect that "if R decides it's a method, then it damned well is a method, whether you think it is or not"?
>
>> - and if so, should there also be a check for functions that look like methods but aren't registered and declared as such?
>
>> My preference would be for the first, FWIW. Admittedly, just because I didn't register 'subset.with.warning' as an S3 method, that won't stop 'subset' from trying to use it if it ever encounters an object of class "with.warning". It's a risk that I'm happy to take, though CRAN might not be...
>
>> I made the warning go away by adding a '...' argument to the end of 'subset.with.warning', but that's not pretty.
>
>> Mark Bravington
>> CSIRO
>> Hobart
>> Australia
>
>>> sessionInfo()
>> R version 2.11.1 Patched (2010-06-30 r52418)
>> i386-pc-mingw32
>
>> locale:
>> [1] LC_COLLATE=English_Australia.1252  LC_CTYPE=English_Australia.1252    LC_MONETARY=English_Australia.1252 LC_NUMERIC=C
>> [5] LC_TIME=English_Australia.1252
>
>> attached base packages:
>> [1] grDevices tools     tcltk     stats     graphics  utils     methods   base
>
>> other attached packages:
>> [1] ad_1.0         chstuff_1.0    handy2_1.2     tweedie_2.0.2  statmod_1.4.1  handy_1.1      debug_1.2.3    mvbutils_2.5.2
>>>
>
>> ______________________________________________
>> R-devel at r-project.org mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel
>
> ______________________________________________
> R-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>
>