adewg / ICAR

Standard messages and specifications for animal data exchange in livestock.
https://icar.org/
Apache License 2.0
49 stars 26 forks source link

icarStatistics API is not ok #353

Closed erwinspeybroeck closed 1 year ago

erwinspeybroeck commented 1 year ago

We wanted to start with this one, but I fear it is not correct

"group": {
  "type": "array",
  "items": {
    "$ref": "../types/icarStatisticsGroupType.json"
  }
},
"statistics": {
  "type": "array",
  "items": {
    "$ref": "../types/icarStatisticsType.json"
  }

Normally the statistics are realted to one group ) I would propose a slightly different construction

"group": {
  "type": "array",
  "items": {
    "$ref": "../types/icarStatisticsGroupType.json",
    "statistics": {
       "type": "array",
       "items": {
          "$ref": "../types/icarStatisticsType.json"
       }
}
cookeac commented 1 year ago

Agreed that @erwinspeybroeck will fix, and we will ask @AlexeyHardCode and @AndreasSchultzGEA to check code generates ok.

AndreasSchultzGEA commented 1 year ago

@erwinspeybroeck: where are the changes available?

erwinspeybroeck commented 1 year ago

The changes are available on the erwinspeybroeck branch. Waiting to create a pull request until the previous one is merged (milk predictions)

cookeac commented 1 year ago

@AndreasSchultzGEA @erwinspeybroeck @AlexeyHardCode I have created a branch which is a release candidate for 1.3.2. https://github.com/adewg/ICAR/tree/RC-for-ADE-1-3-2

This contains Erwin's changes to icarStatisticsResource and icarStatisticsGroupType. I also noticed that icarStatisticsResource did not include allOf icarResource, so have added this (needed to enable use through the generic data exchange API, for instance).

Please test to see if the generated code is ok, and if the group agrees we can then PR this into ADE-1 as a fix release.

AndreasSchultzGEA commented 1 year ago

I reviewed the changes yesterday on Erwins branch and added some comments. I'm not sure, that Erwins adjustments - if any - are included. I'll check the compilation when @erwinspeybroeck gives his ok.

erwinspeybroeck commented 1 year ago

Andreas, where can I see these remarks? I explored my branch via the code, but don't seem to see anything.

AndreasSchultzGEA commented 1 year ago

Ok, they seem to have vanished. then I'll try to post them here:

cookeac commented 1 year ago

We agreed that @erwinspeybroeck will try to make the changes below in the branch: https://github.com/adewg/ICAR/blob/RC-for-ADE-1-3-2/

  • icarStatisticsResource: required "statistics" is not a property any more Remove statistics from the "required" clause.

  • icarStatisticsResource --> "group" (array of "icarStatisticsGroupType"), which contains an array of "statistics" ("icarStatisticsType"). Is this array of arrays wanted? Or is there a mistake? No change required - this is what we want.

  • "icarStatisticsType" contains a "value"-property which looks strange Discussed and agreed that units should just be a "string" and value should just be a "number" (no need to specify double).

  • "icarMetricType" is just an identifier. Why not use the basetype in" icarStatisticsType"? Agreed to keep this icarMetricType but also add an icarMetricType.md markdown file to the well-known folder. This allows people to record their metrics so that they can be used consistently over time.

  • Aren't there a lot of nearly similar type like 'icarStatisticsType' containing value and unit? Why not inherit from them? No change needed. We reviewed and agreed the other measurement types with value and unit were for weights or medicine doses, and not suitable here.