r-devel / r-dev-day

Repo to organize tasks for R Dev Days
7 stars 2 forks source link

Add two new entries to the list of "known good functions with a dot" #34

Open krlmlr opened 4 months ago

krlmlr commented 4 months ago

https://bugs.r-project.org/show_bug.cgi?id=18550

Discussed with Martin Maechler.

mmaechler commented 4 months ago

Well, in the above PR#18550 I pretty strongly argue that as.mcmc.list() should be re-exported by a different name. I think the original package {coda} should also do so (export old and new name with identical definition), and the maintainer should then start to use the new name instead of the old name in all their examples, vignettes, ... Yes, for a time of transition this is somewhat ugly (in coda itself, there's only one method, the default one), but I really don't think it to be "great" if this generic function name is "spreading" to other packages, etc.

To put it (too) drastically: as.mcmc.list is just an abomination for a generic function name. Even Elin got confused about the facts here, and many users will be similarly.

krlmlr commented 4 months ago

I hear that the list of non-S3-methods isn't meant to be changed, typically. I also accept that we don't want to set a precedent. The original problem was a NOTE generated for a package that attempts to reexport coda::as.mcmc.list() . Could we perhaps tweak tools::checkS3methods() so that it sees that mypackage::as.mcmc.list() is actually coda::as.mcmc.list(), which has been allow-listed, and doesn't mention this at all?

The alternative just seems too costly. For the sake of exploration: If {coda} were to create an alias as_mcmc_list <- as.mcmc.list, what should the implementation look like?

Currently:

coda::as.mcmc.list
#> function (x, ...) 
#> UseMethod("as.mcmc.list")
#> <bytecode: 0x118656340>
#> <environment: namespace:coda>

Perhaps this should really be:

# Generic, exported
as_mcmc_list <- function(x, ...) {
  UseMethod("as_mcmc_list")
}

# Default S3 method with entry in NAMESPACE
as_mcmc_list.default <- function(x, ...) {
  as.mcmc.list(x, ...)
}

and with deprecation warnings in as.mcmc.list() at some point.

Then, all users of as.mcmc.list() would switch over to as_mcmc_list(), and all implementers would do so too. The implications for complex class hierarchies are not entirely clear at that point.

This affects 200+ R files on CRAN alone (https://github.com/search?q=org%3Acran+%2Fas%5B.%5Dmcmc%5B.%5Dlist%2F+path%3A**%2F*.R&type=code), with an uncounted number of usages in scripts (1.5k+ on GitHub alone). How much time do we give this problem to converge to the ideal state (the "time of transition")? 10 years might be too short. How many person-years will be spent? The desire to reexport a function doesn't quite justify this kind of effort to me.

Would the effort be justified to clean up the names in the {coda} package? Maybe. But this doesn't seem to be the right place to discuss that change.

CC @elinw .