Open-Orange-Button / Orange-Button-Taxonomy

OpenAPI Orange Button taxonomy
https://Open-Orange-Button.github.io/Orange-Button-Taxonomy
Apache License 2.0
14 stars 19 forks source link

BatterySystem already has a Description property inherited from System #214

Open naulacambra opened 1 year ago

naulacambra commented 1 year ago

BatterySystem inherits from System

      "BatterySystem": {
        "allOf": [
          {
            "$ref": "#/components/schemas/System"
          },
        ....
        ]
     }

And from it, the Description property

  "System": {
    "type": "object",
    "description": "Generic abstract base class for generation/storage systems.",
    "properties": {
      "Description": {
        "$ref": "#/components/schemas/Description"
      },
      ...
    }
  },

But BatterySystem defines again the same property

      "BatterySystem": {
        "allOf": [
          {
            "$ref": "#/components/schemas/System"
          },
          {
            "type": "object",
            "description": "A battery system",
            "properties": {
              ...
              "Description": {
                "$ref": "#/components/schemas/Description"
              },
              ...
            }
          }
        ]
      },

This doesn't raise any problems with the OB's editor or the OpenAPI standard, but it does seem to be unnecessarily redundant.

cwhanse commented 1 year ago

@naulacambra which release of the taxonomy are you looking at? The 2207 release added the inheritance and removed direct use of the inherited elements. In the latest release and the development file, I'm not seeing Description both inherited and defined directly.

naulacambra commented 1 year ago

I'm probably misunderstanding something. I'm looking at Master-OB-OpenAPI.json in the root of the project, in the main branch.

If a schema has the allOf property, doesn't that mean that it inherits the properties of the schemas referenced in the array?

Meaning: BatterySystem, which has an allOf property with a reference to System, and an object with the Description property... https://github.com/Open-Orange-Button/Orange-Button-Taxonomy/blob/7145c0a0eeb977259420b3a97a782d91752c3ac2/Master-OB-OpenAPI.json#L10310-L10337

It has the Description property, defined twice, once by inheritance and once directly, right?

https://github.com/Open-Orange-Button/Orange-Button-Taxonomy/blob/7145c0a0eeb977259420b3a97a782d91752c3ac2/Master-OB-OpenAPI.json#L10164-L10170

The 2207 release added the inheritance and removed direct use of the inherited elements.

I don't see how this release removed direct use of the inherited elements

cwhanse commented 1 year ago

I see your point now. In System there is description (line 10166) and Description (line 10168). I'll have to look into this with someone more expert in json than I am. There isn't a conflict when we make instance files, so perhaps description (at the same level as properties) is a keyword for processing the json file?

The remark about the 2207 release is comparing back to the 2202 release. In 2202, BatterySystem didn't inherit System so had Description (and other objects) on it explicitly. In 2207 we created System as a base class to be inherited by BatterySystem, PVSystem and others.

naulacambra commented 1 year ago

I'm sorry, I probably explained myself the wrong way.

I'm only talking about the Description object inside the properties attribute, not the description attribute itself (note the capitalization).

In the PR #73 Creates Generic System Object, as you said, the System schema was created to group the common properties between PVSystem and BatterySystem.

In this PR is shown that BatterySystem and PVSystem had Description as a property before the changes

PVSystem.Description image

BatterySystem.Description image

And after the changes, System has a Description property System.Description image

BatterySystem still has a Description property BatterySystem.Description image

But PVSystem doesn't have a Description property anymore PVSystem image

With this change, it seems that PVSystem stops having a Description property because now it inherits it from System (which makes all the sense) but BatterySystem still have the Description property defined.

Proposed solution: Remove Description from BatterySystem

cwhanse commented 1 year ago

@naulacambra thanks for being patient here! I see the duplicate Description values now. I overlooked them when I looked at the raw json. And, this has uncovered issues with our editor (openobediter.sunspec.org), which only shows the inherited Description, and our CI checks, which should have picked up this duplication.

jrippingale commented 1 year ago

This needs a fix in the ob-editor