Open-Systems-Pharmacology / QualificationPlan

Creation and processing of a qualification plan
1 stars 2 forks source link

Why are properties required in PlotSettings? #112

Closed msevestre closed 5 years ago

msevestre commented 5 years ago
   "PlotSettings": {
          "type": "object",
          "properties": {
            "ChartWidth": {
              "$ref": "#/definitions/plotSize"
            },
            "ChartHeight": {
              "$ref": "#/definitions/plotSize"
            },
            "Fonts": {
              "type": "object",
              "properties": {
                "AxisSize": {
                  "$ref": "#/definitions/fontSize"
                },
                "LegendSize": {
                  "$ref": "#/definitions/fontSize"
                },
                "OriginSize": {
                  "$ref": "#/definitions/fontSize"
                },
                "WatermarkSize": {
                  "$ref": "#/definitions/fontSize"
                },
                "FontFamilyName": {
                  "type": "string",
                  "enum": [
                    "Arial",
                    "Tahoma",
                    "Times New Roman",
                    "Microsoft Sans Serif"
                  ]
                }
              },
              "required": [
                "AxisSize",
                "LegendSize",
                "OriginSize",
                "FontFamilyName",
                "WatermarkSize"
              ]
            }
          },
          "required": [
            "ChartWidth",
            "ChartHeight",
            "Fonts"
          ]

I think there should not be absolutely Font, or width or height don't you think? Similar to AxisSettings. All opt-in

Yuri05 commented 5 years ago

In the axes settings all fields are mandatory.

msevestre commented 5 years ago

Not according to schema?

Yuri05 commented 5 years ago

thx

Von: Michael Sevestre [mailto:notifications@github.com] Gesendet: Donnerstag, 11. April 2019 13:42 An: Open-Systems-Pharmacology/QualificationPlan Cc: Juri Solodenko; Assign Betreff: Re: [Open-Systems-Pharmacology/QualificationPlan] Why are properties required in PlotSettings? (#112)

Not according to schema?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHubhttps://github.com/Open-Systems-Pharmacology/QualificationPlan/issues/112#issuecomment-482080415, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AX5p9Hf1KZdKDYXn8zNU5T-J8rJvBd0jks5vfx-JgaJpZM4coIdD.

Yuri05 commented 5 years ago

Yep, according to the schema.

I think it makes sense.

msevestre commented 5 years ago

I disagree. According to schema, AxesSettings properties are not mandatory

https://github.com/Open-Systems-Pharmacology/QualificationPlan/blob/master/schemas/OSP_Qualification_Plan_Schema.json#L586

Also the code in the Qualfiication Engine checks for each property before assigning it. In a nutshell, you should not HAVE To define everything in a qualification plan according to me. What ever you define will take precedence over the default in Matlab

Yuri05 commented 5 years ago

AxesSettings are mandatory

AxesSettings for particular plot type (your latest link from above) are not mandatory (because not all plot types are used in a qualification plan). What I would like to achieve (but this is not possible on schema level: if a plot type is available in qualification plan, then axes settings corresponding to this plot type must be given)

Yuri05 commented 5 years ago

Regarding the Plot Settings: it's different discussion for me, than axes settings. Because for the axes settings there are either no defaults (dimension, unit), or it's pretty clear, what should be used (Scaling). For the font settings there are defaults indeed, thus we could make some (all?) properties of PlotSettings optional.

msevestre commented 5 years ago

This is the title of the entry :) Why are properties required in PlotSettings I think this is not helping consistency at all as people will be required to always define something!