Add an even better validator

[peternewman]

With thanks also to @Bartel-C8 !

This is the only one that flags up the fact some PIDs have values of -1!

See an example run here:
https://github.com/peternewman/rdm-schema/runs/2926896569?check_suite_focus=true

This will need #7 in. It also has some unrelated changes from other PRs which will disappear as they are merged.

[ssilverman]

Would you mind splitting this into PRs having related commits? (And related commits squashed together.)

[peternewman]

I'm not 100% sure this needs to be removed, it needs double-checking.

[peternewman]

Would you mind splitting this into PRs having related commits? (And related commits squashed together.)

If you merge #1 and I update, the Codespell stuff will disappear from this.

In terms of the other commits, we'll still need to have @Bartel-C8 's commit in the middle, I've done limited squashing, let alone with other people's commits too.

What do you mean by related (or do you just mean splitting Codespell from JSON validation)?

[ssilverman]

See: #6 (comment)

[ssilverman]

Please split unrelated changes into different PRs.

[ssilverman]

So currently, none of the examples will validate. Here's why (excerpt from the new "Notes on the examples" section in the README):

---

Manufacturer ID zero

All the example messages use a manufacturer ID of zero, even though that will
not validate against the schema. There was some discussion on this and these are
the reasons:

1. It is stated in several places that manufacturer IDs must be >= 1. See:
   1. ANSI E1.20
   2. Control Protocols Working Group - Manufacturer IDs
2. It will ensure that even if someone uses the example messages as a base for
   their own messages, say using cut & paste, they will still need to choose
   their own manufacturer ID.
3. Zero is ESTA's manufacturer ID.

---

Love your input if you would like to share here. It would be relayed to the group next time the group meets.

[ssilverman]

I’ll add: I don’t love that none of the examples validate. Maybe a workaround is to temporarily change the minimum allowed manufacturer ID to zero during the validation step. But I don’t love that either. Still thinking about this.

[peternewman]

I've left my comments in #12 as they felt sufficiently different from this PR (and important enough to be worthy of their own issue rather than buried in a random PR.

I’ll add: I don’t love that none of the examples validate.

Feel free to use one or more of the OLP PIDs as examples if you'd like, their definitions (in the OLA Protobuf format) are here, one or either of us could convert them to the JSON Schema format:
https://github.com/OpenLightingProject/rdm-app/blob/220fdfeaa32b7b9ed745caebde6d2abe0ee09eb9/data/pid_data.py#L2919-L2995

Maybe a workaround is to temporarily change the minimum allowed manufacturer ID to zero during the validation step. But I don’t love that either. Still thinking about this.

Yeah that feels like quite a hack and as per #12 just implies further that they are required. If you want to list ESTA PIDs, then other people probably do (us for starters).

[peternewman]

Please split unrelated changes into different PRs.

I've now resynced this PR with master so it just has the validation commits left in it. Although as you'll see/as you predicted they now all fail due to the examples all being zero based:
https://github.com/ssilverman/rdm-schema/pull/9/checks?check_run_id=3709398398

[Bartel-C8]

@peternewman @ssilverman I think, now that the manufacturer_id issue has been resolved, it would be possible to merge this in as well?

[peternewman]

@peternewman @ssilverman I think, now that the manufacturer_id issue has been resolved, it would be possible to merge this in as well?

Yes mostly @Bartel-C8 .

Queued message fails the validation due to this line:

rdm-schema/examples/e1.20/QUEUED_MESSAGE.json

Line 21 in 2d9d8c4

"get_response": "different_pid"

Plus the draft PIDs with values of -1 fail the validation due to their value.

Could I possibly suggest a rather bold step @ssilverman ? Delete the draft PIDs/standards from the master branch (and fix queued message or I think the schema) and merge this PR in. That way the master branch shows that this schema can validate every currently standardised PID. Then open a PR to restore the draft PIDs, which should pass aside from their PID IDs, making it nice and clear what's going on. Possibly one PR per draft standard, as they're likely to move at different speeds. That proves the draft PIDs are essentially covered by the standard, without including examples for PIDs that might not happen/making the validation fail.

Either that or ask someone to assign test PID values to these draft PIDs as has been done in the past.

[sammysmallman]

Could I possibly suggest a rather bold step @ssilverman ? Delete the draft PIDs/standards from the master branch (and fix queued message or I think the schema) and merge this PR in. That way the master branch shows that this schema can validate every currently standardised PID. Then open a PR to restore the draft PIDs, which should pass aside from their PID IDs, making it nice and clear what's going on. Possibly one PR per draft standard, as they're likely to move at different speeds. That proves the draft PIDs are essentially covered by the standard, without including examples for PIDs that might not happen/making the validation fail.

I support @peternewman's suggestion. I'm glad to see we're now at a stage where the standardised PIDs are validating. Some clarity in separate branches and pull requests for draft PIDs/standards would be welcome.