ropensci / RNeXML

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

Refactors the S4 class structure for meta elements #203

Closed hlapp closed 5 years ago

hlapp commented 5 years ago

This makes the "meta" class, formerly the multiple inheritance subclass, the new superclass of both LiteralMeta and ResourceMeta. The former superclass, "Meta" is eliminated.

This move solves several problems, while avoiding a potential host of others. Specifically:

In addition to the meta class hierarchy, the meta() constructor function is also refactored to try hard to determine the correct type, while also being careful not to overwrite a type that was provided to the call. This will in fact allow a client to create and provide their own metadata class (if they make it inherit from meta or one of its subclasses).

cboettig commented 5 years ago

@hlapp this looks good to me. I think if we just deprecate the simmap vignette and tests as discussed in #201, the checks should pass (?). Then just go ahead and merge #201 & #203 when you are ready.

Also, just a note that #199 looks ready to merge, though in my opinion it would be worthwhile to just make simplify=TRUE the default behavior first. What do you think?

hlapp commented 5 years ago

I think I have a fix for the remaining failure here. I also have an alternative to #201 in the works which would keep the class names. I think I'm at the point where everything passes, just need to document and package it up for a PR.

Also, just a note that #199 looks ready to merge, though in my opinion it would be worthwhile to just make simplify=TRUE the default behavior first. What do you think?

I'm holding that one until after the class name issue is addressed. That's because #201 would make an easy provision to make it the default, and in fact going back to old behavior would be more involved.

hlapp commented 5 years ago

Yay, finally we have a passing test suite now here too. @cboettig I'll take your comment above ("this looks good to me") as approval to merge 😃