S4 Generics and NAMESPACE : justified warning ?
Yohan Chalabi wrote:
Dear list,
It seems that a package (pkgB) using another package (pkgA) with S4
generics formed by taking existing functions (for example 'plot') must
not import the existing functions ('plot') in its namespace to avoid
the warning "replacing previous import: plot".
Suppose we use the simple 'import' directive in the name space of
pkgB.
pkgA and pkgB files would be
pkgA/NAMESPACE:
import(graphics)
import(methods)
exportClasses("classA")
exportMethods("plot")
pkgA/R/pkgA.R:
setClass("classA", contains = "matrix")
setMethod("plot", "classA", function(x, y, ...) NULL)
pkgB/NAMESPACE:
import(methods)
import(garphics)
import(pkgA)
Loading pkgB would then generate a warning because the generic 'plot'
in pkgA overwrites the 'plot' function imported from graphics name
space :
Loading required package: pkgA
Warning message:
In namespaceImportFrom(self, asNamespace(ns)) :
replacing previous import: plot
As far as I understood it, one must explicitly import the functions one
needs in pkgB with 'importFrom' to avoid the previous warning. In
our example, one would then take care to not import the 'plot'
function from garphics name space.
This makes the maintenance of packages using S4 packages rather
tedious because one needs to import exactly what one needs rather than
using the simple 'import' directive.
Moreover, the warning "replacing previous import:" is confusing. In this
case we have a generic replacing its existing function like
'setMethod' would do and not a function replacing another function
with the same name.
Hi Yohan -- Commenting as a user, there's no guarantee that the 'plot' generic defined in pkgA is derived from graphics::plot via setGeneric; pkgA could define it's own generic, and one would want to be informed of the collision. Maintenance of packages that have used simple 'import' to pull in all dependencies is tedious, but using 'import' in some ways undermines benefits of name spaces (restricting the symbol lookup table to reduce the number of symbols and the possibility of name collisions, and to more carefully isolate code inside the package name space to changes in imported packages or induced by the user). So I think a 'better practice' is to explicitly import just those functions, classes, etc that are required by the package. Maintenance of such selective imports is much less tedious, even with complicated package dependencies. There is an unreleased Bioconductor package to identify specific imports, available for R-2.9.* at svn export https://hedgehog.fhcrc.org/bioconductor/branches/RELEASE_2_4/madman/Rpacks/codetoolsBioC or for the development version of R at svn export https://hedgehog.fhcrc.org/bioconductor/trunk/madman/Rpacks/codetoolsBioC with functions findExternalDeps and writeNamespaceImports. These are fairly thoroughly tested, but perhaps not fool-proof. Martin
IMO it is normal that a function in a previous import can be replaced
by its generic. It should not create a warning. But a warning should
be generated when a generic is replaced by another generic.
Does this make sense ?
This could be implemented in 'namespaceImportFrom' with the patch given
bellow.
It would greatly help the maintanance of packages which use S4
packages, because one could use the simple 'import' directive rather
than 'importFrom'.
Looking forward for your comments.
Regards,
Yohan
#################################################################################
Index: src/library/base/R/namespace.R
===================================================================
--- src/library/base/R/namespace.R (revision 49278)
+++ src/library/base/R/namespace.R (working copy)
@@ -797,8 +797,12 @@
}
}
for (n in impnames)
- if (exists(n, envir = impenv, inherits = FALSE))
- warning(msg, " ", n)
+ if (exists(n, envir = impenv, inherits = FALSE)) {
+ if (.isMethodsDispatchOn() && methods:::isGeneric(n, ns)) {
+ if (methods:::isGeneric(n, impenv))
+ warning("replacing previous generic import: ", n)
+ } else warning(msg, " ", n)
+ }
importIntoEnv(impenv, impnames, ns, impvars)
if (register) {
addImports(self, ns,
#################################################################################