Removing old model entity attribute association methods

Simulation Modeling Toolkit (SMTK) Design
1

While working in the exclusion / prerequisite mechanism I came across inconsistencies with the association and disassociation methods. Mainly the removeAllAssociations method calls the model entity specific api and not the more generic persistent object api. I would like to remove the older api in 3.1. If there are objections then we need to at least make sure we use the more general api where possible.

2

Hey, a place we can practice good API migration strategies! Instead of deleting, I’d recommend marking them as deprecated and removing in 4.0. The warnings will guide what needs to be updated, but code will continue to work in the interim instead of forcing a change.

3

I was hoping to support the old API until 4.0 but there is a problem with the existing API. In particular the ability to associate any UUID as an attribute to a model entity doesn’t make sense will the current design. So I propose the existing methods on model::EntityRef will need to change:

  • bool hasAttributes() const;
  • bool hasAttribute(const smtk::common::UUID& attribId) const;
  • bool disassociateAllAttributes(smtk::attribute::ResourcePtr attResource, bool reverse = true);
  • smtk::common::UUIDs attributeIds() const;

To

  • bool hasAttributes(smtk::attribute::ConstResourcePtr attRes) const;
  • bool hasAttribute(smtk::attribute::ConstResourcePtr attRes, const smtk::common::UUID& attribId) const;
  • void disassociateAllAttributes(smtk::attribute::ResourcePtr attResource, bool reverse = true);
  • smtk::attribute::Attributes attributes(smtk::attribute::ResourcePtr attResource) const;

If developers really have a problem with the changes above there is way to support the older API but it might be computationally expensive and will not work if a resource manager is not being used.

Please comment asap cause I would like to fix the existing regressions soon.

4

Is it possible to implement the old signatures and just add on a heavy deprecation message on them? What’s stopping their current implementations from no longer working? They can live along side the new ones just fine due to overload resolution. If it has to change, it has to change, but I think we should be practicing proper deprecation workflows where possible.

5

The problem is that the api on EntityRef I need to change doesn’t take in the attribute resource but instead only uses the model’s internal association information which is now inadequate based on the new constraints. The alternative is to use the resource manager if one exists to find the resource manager which is a pain.

6

If it is something we don’t have development time for, I’m fine with that being a reason. It’s just that API/ABI breaks should be at a higher bar than we’ve been using. The change should be documented and justified in release notes with guidance for the upgrade path.

7

I would change this to:

bool hasAttribute(const smtk::attribute::ConstAttributePtr& attrib) const;

because we are moving to avoid passing UUIDs around.

Otherwise, I think the changes are reasonable.