-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[jts] Refine min* and max* in field constraints #161
Comments
@pwalsh excellent suggestions @ldodds @paulfitz @jpmckinney any thoughts here? |
Re: rename: the names were chosen to match JSON Schema. I think the use of common terms is more important than the slight improvement in clarity. FYI, in JSON Schema, I don't think minimum/maximum makes sense for string, and I don't think minLength/maxLength makes sense for boolean, number, integer. It should just cause an error like "unexpected type 'string' for field with 'minimum' constraint". |
@jpmckinney the thing is here, JSON Schema has very explicit definitions for "validation keywords" (which loosely map to JSON Table Schema doesn't, and, at least for me, that creates some ambiguity. As you note, the existing spec for min* and max* constraints do not make sense for all types. So, if they are going to stay exactly how they are, I think we should:
Still, I do prefer my suggestion above with naming. I'm all for aligning with JSON Schema where possible (like primitive types), but in this case, |
I'm not sure about the rename suggestion, but don't feel strongly either way. I believe we (ODI, etc) do have some schemas using this naming so would prefer to avoid breaking changes. +1 on clarifying the specification further. |
This is my suggestion based on trying to implement constraints for jsontableschema-py. I'm thinking that And For types that don't support a particular constraint, an exception is raised
|
Hey @brew I wholeheartedly agree, and I'm glad to see the issues I originally raised when we started the python lib are raised again now that you are working on it :). I think the way you have implemented it is correct, and we should update the spec to reflect this. Would you be happy to do a pull request on the spec that clarifies the min* max* constraints? |
…pes. Not all field types should support minLength and maxLength constraints. This commit attempts to clarify this.
…aints. Not all constraints support minumum/maximum constraints. Specify the ones that do.
If a constraint is not supported by a field type, the implementation should report an error.
Closing as @brew made the PR and it has been merged. |
A field constraint descriptor supports the following keys related to minimum and maximum values:
minLength
,maxLength
,minimum
,maximum
.The spec describes them as follows:
minLength
– An integer that specifies the minimum number of characters for a valuemaxLength
– An integer that specifies the maximum number of characters for a valueminimum
– specifies a minimum value for a field. This is different to minLength which checks number of characters. A minimum value constraint checks whether a field value is greater than or equal to the specified value. The range checking depends on the type of the field. E.g. an integer field may have a minimum value of 100; a date field might have a minimum date. If a minimum value constraint is specified then the field descriptor MUST contain a type keymaximum
– as above, but specifies a maximum value for a field.I think the intentions are good, but the spec is lacking clarity. The examples don't consider how these may or may not apply to all types. Just thinking out loud, but the following would be helpful for my reading of the spec:
minimum
>minValue
andmaximum
>maxValue
(consistent with the *Length keys, and more explicit in meaning)The text was updated successfully, but these errors were encountered: