Closed cjmyers closed 6 years ago
VariableComponent.getVariants() should return a set of ComponentDefinitions, not URIs.
displayID vs displayId in the CombinatorialDerivation constructors should be consistent.
constructor parameter template is currently URI, but maybe should be ComponentDefintion?
SBOLDocument.getCombinatorialDerivation() should have one with parameters being displayId and version, and not URIprefix
derivation.createVariableComponent() constructors aren't consistent. Specifically, the one with a variableURI should be should be argument #3
VariableComponent.add*() should all/also be able to take in a ComponentDefinition/etc instead of only URIs
VariableComponent.remove*() does not exist. These should return a boolean depending on success.
derivation.getVariableComponent(displayId, version) should exist.
VariableComponent.getVariable() should return a Component, not a Componet URI
VariableComponent.getVariableURI() should return a Component's URI
derivation.removeVariableComponent(URI) methods don't exist
SBOLDocument.removeCombinatorialDerivation(URI) doesn't exist
In general, methods that are get...URI() should return the URI, and get...() should return the Object.
@cjmyers These are the inconsistencies I've found while using the new classes. You know the library better than I do, so feel free to correct me if I'm wrong.
@IgorDurovic Please defer to Chris :)
I agree with most of these, except perhaps:
With createComponent, we don't have this option, so it would be okay to only have URI option. Granted, I see it is useful, so maybe in future we should add this option to all with references to a topLevel.
Same as last comment.
Another thing I found:
derivation.isSetStrategy()
VariableComponent.setOperator(OperatorType)
createCombinatorialDerivation should not take strategy, since it is an optional parameter.
I fixed the createCombinatorialDerivation and corresponding constructor.
Ah, I think I have found the culprit!
fromDoc.createRecursiveCopy(toDoc, derivation);
derivation has a variable component with variants. This derivation is contained within fromDoc. After the call, toDoc does indeed have the derivation and variable component, but the variable component doesn't have any variants anymore. I haven't checked if it preserves nested derivations or variant collections, but I would assume it's the same code path.
Once this is fixed, serialization should also be fixed (it may not have been the original problem, but Chris's fixes do seem to match better with the other objects).
Completed
See SEP #007
https://github.com/SynBioDex/SEPs/blob/master/sep_007.md