SynBioDex / SBOL-specification

The Synthetic Biology Open Language (SBOL)
http://sbolstandard.org
13 stars 9 forks source link

Add SBOL3 validation rules #417

Closed cjmyers closed 3 years ago

cjmyers commented 3 years ago

This PR includes all the SBOL3 validation rules along with some corrections in the SBOL3 specification discovered when creating the SBOL3 validation rules. This needs a thorough review before merging from:

SBOL3 library developers: Tom, Goksel, James: please read over all the rules, and let me know if they make sense to you.

SBOL Combinatorial Designers: Nic, Jake: I need you to review CombinatorialDerivation and VariableFeature. The provenance rules that Nic added are tricky, and need another set of eyes on those. They are also not described in the specification body and likely should be. I need Jake to make sure I incorporated the rules from his SEP correctly. They are likely not complete.

SBOL Provenance: Bryan: could you look over the Prov-O rules, especially those related to DBTL?

SBOL Measures Nic: can you verify the OM rules?

General Formatting and Consistency Pedro/Prashant: can you look over the general formatting and consistency?

jakebeal commented 3 years ago

I'd like to compress most of these rules into two meta-rules and a table.

My observation is that the vast majority of rules are all instantiations of one of two meta-rules:

If we put these up top, then (a) the size of the validation rules section would drop dramatically, and (b) most of the implementation can be in the form of a table rather than code, which is much easier to debug and maintain.

cjmyers commented 3 years ago

I don't mind adding a table, but I still like the list. The list was actually copied in to libSBOLj to give the precise messaging. Also, I've found it very useful to be able to see everything for one object at a time in one place. Finally, this table would be enormous, so you really would need a table per class or so.

jakebeal commented 3 years ago

I'm actually thinking that maybe the table could be in the form of something like an RDF document, rather than prose, which could then be excerpted into the specification. Would you mind if I tried putting together an example of what I'm thinking, and see if it could work for you? The really key thing I'm aiming to accomplish is to separate out the "special" rules for each class from the routine type/cardinality information that can obscure it.

cjmyers commented 3 years ago

Sure it would be fine to take a stab at this. I'm still not entirely sure about replacing the list, which I find very useful as explained above. However, at the very least, having a more machine friendly version could be good.

Please do keep in mind that I strongly believe libraries should do more than just check documents after generation. I really think this makes code debugging extra difficult. There should be checks done by the setter functions to ensure that invalid entires are caught at creation.

jakebeal commented 3 years ago

A question I've found: our text and diagram disagree whether SequenceFeature:hasLocation has cardinality [0..n] or [1..n]. I think it is intended to be [1..n], but I'd like a second opinion before I fix it.

bbartley commented 3 years ago

Hi @jakebeal I'm not sure if a Location on Feature should be or is intended to be required. There are use cases in which one might want to express relative spatial relationships via Constraints rather than explicit Locations

cjmyers commented 3 years ago

I thought we decided that SequenceFeature would not make much sense with relative constraints. Part/sub-Part certainly, but not sure if there is use case for a SequenceFeature to be relative.

bbartley commented 3 years ago

OK, I see, we are talking about SequenceFeature and not Features in general. I have no quarrel with this then.

jakebeal commented 3 years ago

OK; I will make that change then.

jakebeal commented 3 years ago

@cjmyers I've got a pull request (#424) opened to add my revisions to date into this branch. Can you please review and accept if satisfied?

jakebeal commented 3 years ago

I believe this is obsolete now, having been rendered obsolete by #424. @cjmyers Can you please confirm and close?