cityjson / specs

Specifications for CityJSON, a JSON-based encoding for 3D city models
https://cityjson.org
Creative Commons Zero v1.0 Universal
108 stars 25 forks source link

Should "extensions" have a version property for every listed extension? #44

Closed liberostelios closed 5 years ago

liberostelios commented 5 years ago

Currently, the "extensions" property lists the URLs of the definitions of the extension contained in a CityJSON file. While the definition of the extension contains a "version" property, the file doesn't specify explicitly which version of the extension it is implementing.

Obviously, the URL points to a definition of a specific version of an extension, therefore one can argue that it might be redundant to add it in the file itself. If I am pointing to the URL of Noise extension v0.1, why should I specify it in the file itself? From a developer's perspective, though, having to fetch and parse the definition file in order to determine the version of the extension that the file is using adds a lot of unnecessary complexity (and overhead).

I suggest that the list of extensions should have not only the URL, but the version of the extension that this file implements. This way, it's easier for a piece of software to validate whether it can parse a file or not.

For example:

"extensions" {
  "Noise": {
    "url": "https://someurl.org/noise.json",
    "version": "0.1"
  }
}

Btw, shouldn't the "extensions" property be an array instead of an object?

hugoledoux commented 5 years ago

I agree that the version of the Extension used should be explicitly mentioned, I'll implement this for v1.0.0

Should the "extensions" be an array? Hmmm, you mean it'd be like this?

"extensions":[ 
  {
    "name": "Noise",
    "url": "https://someurl.org/noise.json", 
    "version": "0.1"
  },
  {
    "name": "Potato",
    "url": "https://someurl.org/potato.json", 
    "version": "0.1"
  }
]

I'm not against, but does it really make it better? If you think yes I'm happy to make that small change.

liberostelios commented 5 years ago

From a developer's view, I suppose everyone wants to iterate through all the listed extensions under "extensions", so an array fits better.

kenohori commented 5 years ago

Isn't iterating over object members just as easy?

hugoledoux commented 5 years ago

it is, but this version makes it clearer.what the name of the extension is I think. Although that can be fetched from the file... But I somehow prefer it.

liberostelios commented 5 years ago

@kenohori I suppose most (if all) JSON parsing libraries do make it effortless to iterate through object members. But out of intuition I don't find iterating through an object's members a good practice.

Ironically, we are doing it for city objects, but in their case the benefit of key-value pairs is more obvious than for extensions.

kenohori commented 5 years ago

Agreed that it's less obvious, but it's still probably easier to check for the files with a given extension if it's an object. Something like dataset["extensions"].exists("Noise") is more user friendly than an iteration IMO.

liberostelios commented 5 years ago

That's a good point, actually.

There is a downside, though. I think that more attention should be paid on how the extension names are created. For instance, we should stress to people that the name of an extension should not be as broad and vague as "Noise". The name should be more descriptive in a way that multiple people can make their own Noise extensions which would co-exist without conflicts.

There are pros and cons in both approaches. I have no strong opinion on the matter, but I find the array approach more "safe".

hugoledoux commented 5 years ago

The code of cjio was already accessing Extensions based on their names (with an array it'd be a number, more cumbersome for programmers I reckon) and thus I kept the dictionary but added the version as mandatory.

And I added this to the specs, which was missing!

https://github.com/tudelft3d/cityjson/commit/ed280ffa4cad12e9821596f09fc6373fcd5f8d60