Policy: rule_unit is optional but does not define a default

[ascherkus]

Describe the bug
Most of the optional policy fields specify what the default value is when not present (which is fantastic!)

However rule_unit is optional, but does not specify what the default behaviour is.

To Reproduce
Steps to reproduce the behavior: N/A

Expected behavior
I'd expect either:

1. Clear rules about what the default behaviour should be
2. rule_unit to be required depending on rule_type

In general, I lean towards explicitness in this instances so my vote would be (2).

I worry about the complexity of (1) growing over time, especially since we've already introduced metric vs. imperial units (kph and mph). To keep it simple we'd either have to:
a) Remove kph (or mph 😉)
b) Stick to a single unit globally (e.g., kph) as the default
c) Go down the path of having the default change based on locale (e.g., default rule_unit is kph if these policies are for regions in Canada)
d) Some other idea that I haven't thought of off the top of my head

Again, I think (2) with some tests as well as a proper schema (see #389) would go a long way to clarifying things.

Additional context
Came across this while prototyping a tool for #423.

Happy to send out a PR - just want clarification before doing so!

[avatarneil]

Hi @ascherkus! When designing Policy, we were simultaneously working on policy endpoints to be contributed to mds-core. As you've found, there are definitely some issues with types within the Policy spec itself -- likely sprouted due to our focus on the expression of Policy in code. We use wrote mds-core in Typescript, which let us write very prescriptive types for Policy and Rules. You can see how we expressed this here take note of BaseRule and how we extend it into CountRule | SpeedRule | TimeRule.

Re: #389, we are able to generate json schemas based on our typedefs, but are still working on finding a nice way to tie that in with the existing schema generation tool that's in the MDS repo. Our intent is to cut a PR for that at some point. In the meantime, a sneak preview for generating the schemas...

1. Install https://github.com/YousefED/typescript-json-schema (I've been installing it globally)
2. Navigate to mds-core and run typescript-json-schema packages/mds-types/index.ts Policy > policy_schema.json

I'm not sure of the best way to express this in the spec, so open to suggestions.

[ascherkus]

Thanks for the pointers!

IIUC, based on the typescript definition it looks like rule_unit is required when rule_type is either time or speed. I think it should be possible to write text description of the typescript types you've linked to if my understanding is correct.

I've got a related, broader question I'll ask the mailing list re: the JSON schema definition bit.

[quicklywilliam]

Noting that our PR for #472 will contain some additional rule_units. I want to be sure to keep the new units inline with whatever the planned resolution of this issue is. Anything I should do now?

[marie-x]

units was originally optional because count has only one type (vehicles) and so for that rule_type the unit is superfluous. In the interests of rigor, I am inclined to add the vehicles type and make the field required. If there's consensus around this, I will write a PR.

[schnuerle]

@Karcass Did a PR get made for this, and if not, should we move to 1.1.0?

[schnuerle]

Working to resolve this now with #539.

[schnuerle]

Most of this is resolved with PR #539. One thing that wasn't was picking just one speed per time unit, and kept both mph and kph.

Please open another issue @ascherkus after you review all the changes in context if you'd like to revisit something.