IDs & relationships for #181

[dselman]

Issue #181

Rework identifiers and relationships. See issue for details and design.

Changes

• PEG grammar updated
  • allow identified by for Concepts
  • add support for identified — automatically create an identifying field called $identifier
• Remove Concept from the mode (Resource and Concept are now functionally identical)
• Update Factory to always delegate to newResource when newConcept is called, checking whether the Concept has an identifying field
• Allow relationships to any class that has an identifying field (reject otherwise)

Flags

• Nothing yet to automatically generate IDs. Should that be a feature of the Factory?

[jeromesimeon]

Looks great so far. Only one requested changes (version in package.json) and a few questions/comments.

I think some more testing, etc. but a great start. Couple of open questions:

• allow asset/participant/events/transactions without identifier or not?
• prevent "inlined" form for identified classes? (i.e., force having a relationship to them?).

[dselman]

Looks great so far. Only one requested changes (version in package.json) and a few questions/comments.

I think some more testing, etc. but a great start. Couple of open questions:

• allow asset/participant/events/transactions without identifier or not?

I think we should prevent non abstract non identified asset/participant/events/transactions. It is a breaking change...

• prevent "inlined" form for identified classes? (i.e., force having a relationship to them?).

Here I am inclined to say that we should allow containment of identified classes. If person A creates a model using asset they can't know whether person B would like to use it with a containment or using a relationship. I've seen quite a few models that would like to store a collection of transactions, for example.

[dselman]

Not sure why ObjectValidator coverage has been lowered by these changes. I need to look into that.

[jeromesimeon]

Looks great! The tests for handling identifiers in various classes make sense (and I think look good from a user point of view).

From where I stand this looks good to go as soon as the coverage gets back up.

The coverage drop seems to be related mostly to error cases not being triggered. https://coveralls.io/builds/31536708/source?filename=packages%2Fconcerto-core%2Flib%2Fserializer%2Fobjectvalidator.js#L392

I'm unsure if it means some tests need to be changed/added or if those errors have become essentially useless (are dead code with the new identifier/relationship support).

[jeromesimeon]

Looks great.