Modifications to extensions per #103.

[bhilburn]

This branch makes the changes described in #103, including:

1. Move the extensions list to an actual JSON array
2. Break version out from optional.

I don't really care for the naming of the fields in the extensions objects at the moment. I first made them without the ext_ prefix (e.g., core:name), but then decided against it as it creates a name conflict with core:version in the parent global object. I'd love to hear thoughts & feedback on this topic.

Note this PR also contains a few other issues to fix some MD linter warnings, which is a bit sloppy to wrap up in this branch - my apologies.

[djanderson]

I actually don't think we need to prepend core: or ext_ to the keys in the extension object. The core:extensions key puts the entire object in the core namespace and the key names are only allowed within that object, not at the top level, so they'll never conflict with existing fields.

In other words, you can really think of these fields like metadata["global"]["core:extensions"][0]["version"], not as something that exists outside of that tree.

[n-west]

That decision should probably be bigger than just the extension object fields. Right now I don't know of any other field that has an object type for value. I think I would lean away from names that cannot be directly used as variable names in common languages (python, c, c++) but we've already crossed that threshold. I think there should be some higher level guidance that values of object type either always use the parent namespace or that object values don't use a namespace.

I need to look at my C++ serializing tools to figure out what would be most sane to support before offering further suggestions, but even that would just be based on my preference because of tools I already have that I'd rather not be broken by a decision like this :-).

[djanderson]

Here's an example to make my point:

import json


class NamespaceExtension(object):
    """Type used to deserialize core:extensions objects."""
    def __init__(self, name, version, optional):
        self.name = name
        self.version = version
        self.optional = optional


# Some example metadata
json_md = """{
    "global": {
        "core:extensions": [
            {
                "name": "extension-01",
                "version": "v0.0.5",
                "optional": true
            },
            {
                "name": "extension-02",
                "version": "v1.2.3",
                "optional": false
            }
        ]
    }
}"""


if __name__ == '__main__':
    md = json.loads(json_md)
    for ext in md["global"]["core:extensions"]:
        try:
            deserialized_ext = NamespaceExtension(**ext)
        except Exception as err:
            print("Invalid core:extensions object - {}".format(err))

        fmt = "Extension {0.name} ({0.version}): optional: {0.optional}"
        print(fmt.format(deserialized_ext))

Outputting:

Extension extension-01 (v0.0.5): optional: True
Extension extension-02 (v1.2.3): optional: False

Note that if we prepend core: to the names, they would not map cleanly to variable names that we can use in most common languages (as @n-west mentioned). There is no chance of confusing the keys inside the core:extensions object with keys in other keys named the same thing in other objects, so adding ext_ doesn't add any information. Try rewriting the code above to use ext_ and it will be either harder to deserialize or not read as cleanly.

[bhilburn]

Okay, I think the resolution, here, is to update this PR to not prepend core to every subfield, and that it should look like the code in @djanderson's snippet, above. Specifically:

{
    "global": {
        "core:extensions": [
            {
                "name": "extension-01",
                "version": "v0.0.5",
                "optional": true
            },
            {
                "name": "extension-02",
                "version": "v1.2.3",
                "optional": false
            }
        ]
    }
}

Will tag this for me

[jacobagilbert]

This is a solid improvement. Once this is merged it probably makes sense to re-evaluate all of the required <EXT>:version fields in the gloabl namespace.

[gmabey]

@bhilburn Is this PR truly ready to merge?

[bhilburn]

@gmabey - No, I have not made the changes referenced in my previous comment, yet.

[jacobagilbert]

@bhilburn this has been rebased and updated per discussion above. The last thing we need to do before merge is to remove the individual global scope version fields in the canonical extensions. I am in favor of completely removing them in v1.x (technically not backward compatible with v0.x but 'safe' still as even if they were used they will just be ignored). That OK w/ you?

Are adsb, rfml, and wifi considered canonical extensions? Or just extensions included here for convenience? I'll add them in if they are.

Whats up with volatile? Its just an empty file and I don't see a need to keep this around in its current state.... Seems like multirecordings is actually going to cover what this was intended for?

[bhilburn]

@jacobagilbert - Oops, thought I had already responded to this.

• volatile is a ghostfile, will be removed in Multiple Recordings Extension #151 merge.
• All other extensions /are/ canonical and should be updated accordingly.

If you don't mind updating them / adding them to this PR, we should be good, here

[jacobagilbert]

@bhilburn this dropped off my queue... but should be good to go now. I also took this opportunity to add section numbering to each canonical extension.