nexml_read functions complains about "XMLInternalDocument" as an input

[xu-hong]

If I give an XMLInternalDocumentobject as input x, the nexml_read(x) will raise: Error in as.vector(x, "character") : cannot coerce type 'externalptr' to vector of type 'character'

This is because the first if condition uses grep("^https?://", x), which do not accept an XMLInternalDocument type.

Should consider reordering the sequence of if statement. @hlapp

[hlapp]

@sckott and @cboettig: for context, the idea is that fixing this would allow the return value from httr::content (with the NeXML document returned by the Phenoscape API) to be passed on directly to nexml_read, instead of having to create a temporary file first. Can you confirm that this is indeed how it would work if the above issue were corrected?

[sckott]

Hmm, one option would be to make an S3 generic and dispatch on different inputs instead of using if..else

e.g.,

foo <- function(x) {
    UseMethod("foo")    
}
# for URL and file name inputs (then just need logic to act appropriately on url vs. file
foo.character <- function(...)
# for XMLInternalDocument input
foo.XMLInternalDocument <- function(...)
# for XMLInternalNode input
foo.XMLInternalNode <- function(...)
# any others ...

[hlapp]

Hmm, one option would be to make an S3 generic and dispatch on different inputs instead of using if..else

Is if...else a discouraged way of doing this? Or are you saying that there is more to it than just changing the order of the conditions to make the problem go away?

[sckott]

The way I proposed is just another way of dealing with various inputs. In the case where a function can have lots of different inputs, the above seems a little bit cleaner, thoughts @cboettig

[cboettig]

@sckott sure, that looks like a good way to deal with the issue and make the code cleaner. @hlapp Scott's approach just relies more explicitly on classes, I think it should also deal with the problem based @xu-hong 's comments. Haven't had a chance to dig into this, but PR welcome.

[sckott]

it's coming

[hlapp]

I think this can be closed as per #131 and @xu-hong's confirmation.