Should notes only be of type array?

[Elchi3]

Noticed another small inconsistency between WebExtension data and CSS data (working on a macro really helps).

WebExtension data always uses arrays even if there is only 1 note in notes. I think this is better from a consumer's perspective. The name notes also implies this I think (notes not note).

The docs say "notes is an array of zero or more translatable string containing additional pertinent information. If there are only 1 entry in the array, the array can be ommitted". I don't know the motivation behind this.

Proposed schema change:
Old:

  "notes": {
          "anyOf": [
            {
              "type": "string"
            },
            {
              "type": "array",
              "items": {
                "type": "string"
              }
            }
          ]
}

New:

"notes": {
  "type": "array",
  "items": {
    "type": "string"
  }
}

@wbamberg @teoli2003 thoughts?

[wbamberg]

We discussed this in Toronto.

I think the version of the schema that we looked at in Toronto had this array-or-string dual form for support_statement but not for notes. There was discussion about whether support_statement should have this dual form. It was decided that it should, then someone said, shouldn't notes have the dual form as well, so as to be consistent.

I agree that having only one form makes the schema and the parsing code both simpler, so that's a good argument in its favour. But having already made the decision one way before, we should be careful we're not overlooking any arguments on the other side. And should we do the same for support_statement?

Note that I'm inconsistent too: the WebExtensions macro assumes that support_statement is a single simple_support_statement object, which works because all the WE data is currently single simple_support_statement. But it shouldn't, really.

[stephaniehobson]

Since we're not putting any code in the notes, supporting multiples statements is a straightforward way to separate notes on different topics.

[Elchi3]

I'm not sure I buy the consistency argument and support_statement is a completely different story to me. Generally, I think having more options will lead to more code, and I think making notes always arrays, would reduce access complexity for at least this property.

[wbamberg]

FWIW I agree with you, although I don't feel strongly either way.

[Elchi3]

Now that the macro implementation is done for both paths, I don't have any strong feelings anymore either 😆 This would be a good issue for feedback from other consumers, but unfortunately we don't have any (known) users of the data yet.

[SebastianZ]

Late feedback from my side, though I agree with @Elchi3's initial argument that notes written in plural form is an indicator for only allowing arrays. Looks like that's the only argument that's still valid after the macro implementation now supports both.

Sebastian

[a2sheppy]

I feel that it should always be an array, for consistency in part, but mainly because it simplifies code paths when interpreting the data, reducing the chances for bugs to occur.

[connorshea]

It maybe isn't a big enough deal to warrant changing all the files, but it'd always be preferable from the consumer side of things for the JSON to have only one data type if possible.

For my BCD Explorer, I have to write code like this when parsing the notes:

if (jsonKeys.includes('notes')) {
  if (Array.isArray(json.notes)) {
    contentForPopover.set("notes", `Notes: ${json.notes.join("<br>")}`);
  } else {
    contentForPopover.set("notes", `Notes: ${json.notes}`);
  }
}

when I could be writing it something like this:

if (jsonKeys.includes('notes')) {
  contentForPopover.set("notes", `Notes: ${json.notes.join("<br>")}`);
}

[Elchi3]

We've been living with the two paths for a long time now and accept "string or array" for other properties as well, so I think we won't fix this after all.