search

ssilverman / rdm-schema

The schema for the Parameter Metadata Language from Section 5 of E1.37-5.
10 stars 3 forks source link

Add an even better validator #9

Open peternewman opened 3 years ago

peternewman commented 3 years ago

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.

peternewman commented 3 years ago

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 commented 3 years ago

See: https://github.com/ssilverman/rdm-schema/pull/6#issuecomment-888343344

ssilverman commented 3 years ago

Please split unrelated changes into different PRs.

ssilverman commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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

peternewman commented 2 years ago

@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: https://github.com/ssilverman/rdm-schema/blob/2d9d8c44c96ff44f337868743ce6fdaf303338eb/examples/e1.20/QUEUED_MESSAGE.json#L21

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 commented 2 years ago

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.