Restrictions on keys in medium metadata and annotation properties

[angus-lherrou]

#60 (at 5a53b53) implements Annotation.add_property and Medium.add_metadata using setattr (the previous code, which appeared to hint at __setitem__, was not yet implemented). This necessarily requires that all properties in the annotationProperties and mediumMetadata JSON objects be valid Python identifiers, and not just valid strings as before.

If this is a requirement that we are okay with, we can proceed with the change I've implemented in that commit. If we'd prefer not to have that restriction, we can implement it differently (i.e. with a field in the AnnotationProperties and MediumMetadata classes containing a dictionary and some magic methods to wrap around that).

[keighrim]

I like the idea of using setattr and isidentifier. However since users (devs of individual tools) can shove pretty much anything in the metadata object (e.g. a metadata key can start with a numeral), so unless we have a robust way to perform round-way string conversions (e.g. @ <-> _), I think we might need to keep it as a old plain dict.

[angus-lherrou]

I agree with you. I can implement that in #60 or in the branch I'll create for #62 tomorrow.

Do you think we should retain the AnnotationProperties and MediumMetadata classes in that case as wrappers for dicts, or just use plain dicts?

[keighrim]

Having them as python classes is still good idea, I think, so that we can have control over reserved fields (some required, others optional).

[keighrim]

@angus-lherrou ba0d546 attempts to solve some issues from having two-tiered attributes in a class, namely some named attributes at first-class, and a dict for free-for-all optional (second-class) attributes (particularly here and here in the code). The very problem I want to solve is when all fields of a MMIF object are optional, we don't want that object serialized as {"XXX": {}} or {"XXX": null}, but just want to ignore that. I though that might be related to this issue as well, and if wanted make sure if you have written something similar or maybe a better solution to that

[angus-lherrou]

That makes a lot of sense. I hadn't thought about the all optional fields issue, so that's great, and I had been thinking along the same lines as far as having required attributes as first-class attributes.

One potential issue with the way you're handling private fields is that arbitrary user-defined attribute names might have trailing underscores, and those would disappear on serialization. So we could tell users about that behavior and restrict possible attribute names, or we could use a different way (maybe a "private_attributes" dict in every such object).

[keighrim]

Maybe we can move that logic up to the superclass MmifObject so that any MMIF object can have optional custom attributes.

Your point on the private marker is very valid, I think. I added that only to handle name conflict btw text field and setter function fortext.@value, and I'll revert the marker code and find another way to resolve the conflict.

[keighrim]

ba0d546 (rebased) is generalizing the idea of two-tiered class attributes. I think we might want the named vs unnamed attribute groups in the MmifObject super class so that any subclass can use the same logic (maybe a flag for disabling unnamed group would be a plus for those subclasses - e.g. annotation - don't want them). But that should be after merging #60.

[keighrim]

#82 implements naming constraints on keys in MMIF object (basically by not posing any constraints). As we're happy so far with the implementation, I think this issues can be closed now. I'll let @angus-lherrou to confirm.