search

clingen-data-model / clingen-interpretation

Allele (variant) interpretation model and API for ClinGen
3 stars 1 forks source link

Do we need a separate _<Type>Attribute table for each type? #187

Closed bpow closed 6 years ago

bpow commented 6 years ago

At some point, IdentifierSystem attributes were split out from the sheet they are in to have an _IdentifierSystemAttribute table. When this happened, I had to make a reference to this in reformat_examples.rb (commit 03ed9e6)

However, the only reason we have needed separate "join tables" like this is to allow for attributes that are not 0..1 or 1..1. All of IdentifierSystem's attributes are 0..1 or 1..1, and I think it is easier to edit such attributes (and to make sure that they don't accidentally get multi-valued) if they are included as part of the main sheet for the instances.

Maybe @larrybabb is the only one who might want to comment here.

larrybabb commented 6 years ago

I can merge it all back. I was thinking that the X _XAttribute pattern was ideal in the event a new multi-valued attribute came about.

@bpow I don't mind putting the IdentifierSystem attributes back on the IdentifierSystem sheet as columns. You please tell me your preference. My perception from reading above is that it makes things easier.

I won't do anything until you give the final word.

bpow commented 6 years ago

I thought we were at the point of not adding lots more attributes 😄

I can code reformatexamples.rb to be more robust in looking for the pattern Attribute, but hadn't done so in part because I thought we were so close to done...

I kinda' liked it back in the old days when only the multi-valued properties were in join tables, but I certainly wouldn't go through the process of moving everything back just to move it back now, and having attributes consistently separated out the way they are may simplify your sheets formulas.

The reason this came up was I was in the process of pulling iris from IdentifierSystem-- if the prefix and iri attributes had been in the IdentifierSystem sheet itself, I could have just pulled the data directly from data/flattened/IdentifierSystem.json-- instead I reworked construct_context.rb to use reformat_examples, so this worked after I added the _IdentifierSystemAttribute reference there.