ropensci / RNeXML

Implementing semantically rich NeXML I/O in R
https://docs.ropensci.org/RNeXML
Other
13 stars 9 forks source link

clash with "tree" class name provided by cli package #169

Closed fmichonneau closed 5 years ago

fmichonneau commented 5 years ago

Trying to load this file (from OpenTree), I get:

Found more than one class "tree" in cache; using the first, from namespace 'cli'
Also defined by ‘RNeXML’
Found more than one class "tree" in cache; using the first, from namespace 'cli'
Also defined by ‘RNeXML’
Error in validObject(.Object) : 
  invalid class “MethodWithNext” object: invalid object for slot "nextMethod" in class "MethodWithNext": got class "NULL", should be or extend class "PossibleMethod"
cboettig commented 5 years ago

yup, I've seen that as well ... unfortunate. Do you need to have cli package loaded?

in RNeXML, the S4 classes match the NeXML node names, so there's no obvious way I see to avoid this. (even if we broke the correspondence, it would be a compatibility-breaking change).

um not directly related but possibly still relevant to you -- last year at a hackathon a couple of us drafted https://github.com/cboettig/nexld . technically it's a converter between NeXML and JSON-LD, but the JSON-LD version corresponds very naturally to R lists / S3 classes. I think that ultimately that would provide a much easier way to work with NeXML in R; objects could be created and parsed as lists and rendered as NeXML or JSON-LD, whatever you prefer.

The JSON-LD approach is particularly appealing, because it makes all NeXML elements, and not just the "meta" elements, into RDF / semantic data too. Just needs a little work to flush out the functionality still (help welcome!)

fmichonneau commented 5 years ago

I didn't investigate too much, but I think cli is loaded along with testthat. I got this error as part of the unit tests.

The nexld approach looks cool. Maybe @mtholder would be interested...

cboettig commented 5 years ago

Yup, cli gets loaded when testthat is loaded these days. I think the error is sensitive to the order you load the packages; loading RNeXML first should avoid this. in any event it's a nuisance to developers but I'm not entirely sure we can do anything to avoid it.

hlapp commented 5 years ago

I think this is an issue that we need to fix. The cli package is now loaded whenever devtools is loaded, and this will prevent proper operation of some of the main functions of the package:

> library(devtools)
> library(RNeXML)
Loading required package: ape
> nex <- read.nexml(system.file("examples", "trees.xml", package="RNeXML"))
Found more than one class "tree" in cache; using the first, from namespace 'cli'
Also defined by ‘RNeXML’
Found more than one class "tree" in cache; using the first, from namespace 'cli'
Also defined by ‘RNeXML’
Found more than one class "tree" in cache; using the first, from namespace 'cli'
Also defined by ‘RNeXML’
Error in validObject(.Object) : 
  invalid class “MethodWithNext” object: invalid object for slot "nextMethod" in class "MethodWithNext": got class "NULL", should be or extend class "PossibleMethod"

The simple fact of calling library(devtools) prior to loading RNeXML essentially results in one no longer being able to load a NeXML file (unless, actually, that file doesn't include a tree, but I don't think that's any consolation).

It seems that the only way to circumvent that is to either

The latter assumes that the problem is only the call to new("tree") in the method implementing coercion from XML to the "tree" class. I'm not sure this is actually the case; when I tried it I did not succeed in fully eliminating the issue (and I could not see another place where new("tree") was being called, but I may have done it incorrectly too since this resulted in another error also.

BTW the "tree" class is not the only one that clashes (see epurdom/clusterExperiment#66). And it's rather sad that S4 classes don't bring any advantage here over the S3 system of generics. (Why can't a package not be provided in the call to as(), as it can be to getClassDef()?)

cboettig commented 5 years ago

I think the problem is the class definition itself; we'd need to change the name of the R class (and adjust the serializing/parsing routines to handle this, since they currently exploit a 1:1 correspondence between element and class names.

And yes, agree completely that it's sad S4 doesn't really help here. In general I think S4 classes are not all that suited for this use case; they give us inheritance but the assumption seems to be that you will have a small number of S4 classes that are designed to be generic types across a large number of packages.... In nexld everything is just S3 classes anyway...

hlapp commented 5 years ago

@cboettig I have a change that would address this for the foreseeable future. If you can at least bless #200, I can rebase and submit for pull request.