Skip to content
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

Correct number of primitive types from seven to six. #145

Closed
wants to merge 1 commit into from

Conversation

kw217
Copy link

@kw217 kw217 commented Nov 16, 2016

The core spec lists six primitive types in section 4.2 (null, boolean, object, array, number, string), but this sentence claims there are seven. Correct it to match.

I'm not sure if this is the right fix - it's also possible that seven is correct, but the reference to the core spec is wrong. Presumably "integer" is meant to be allowed here too - but the type "integer" is nowhere defined - not in the core spec, or the validation spec.

The core spec lists six primitive types in section 4.2 (null, boolean, object, array, number, string), but this sentence claims there are seven. Correct it to match.
@epoberezkin
Copy link
Member

epoberezkin commented Nov 16, 2016

integer is used in validation spec, so rather than just fixing the number it should say 6 primitive types or "integer"

Alternatively integer can be added to core and become "primitive type"

@awwright ?

@kw217
Copy link
Author

kw217 commented Nov 16, 2016

Just adding or "integer" isn't enough, because the spec needs to say what integer means. I think that belongs in the core spec; after all, the core spec already talks about "integer JSON numbers" (5.3) and has an example that mentions type: "integer" (8.2.1).

@@ -552,7 +552,7 @@
an array, elements of the array MUST be strings and MUST be unique.
</t>
<t>
String values MUST be one of the seven primitive types defined by
String values MUST be one of the six primitive types defined by
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the

Recall: "number" includes "integer".

bit on line 560 also be removed? Since there is no longer an "integer" primitive type, that sentence is a bit confusing.

@handrews
Copy link
Contributor

@epoberezkin "integer" was removed from the types in core, so if it's referenced anywhere else in validation that is also a bug and should be cleaned up. Or we need to restore it to the list in core, but I think @awwright wanted to stay with the actual JSON primitive types instead.

@epoberezkin
Copy link
Member

What is the motivation to remove "integer" from validation types? When/where was it discussed?

@epoberezkin
Copy link
Member

@handrews @awwright The reason to have integer as a special validation type is that integers are more commonly used than any other number. Without "integer" you would have to use {"type": "number", "multipleOf": 1} which is very weird.

@handrews
Copy link
Contributor

@epoberezkin - @awwright removed it at some point, presumably before I was here as I don't recall any issue or discussion.

I'm in favor of "integer" as it is an extremely common case and annoying to do with multipleOf and/or a $ref. I just had not gotten around to asking what happened to it.

If we are getting rid of integer, it's still used in an example in the core spec so that should be fixed, too.

@epoberezkin
Copy link
Member

All such changes will eventually lead to the fragmentation of what the standard means and as the result some other standard will be developed. None of the validators is going to drop the support of integers. And yet the standard will not have it. That's ridiculous even as proposal, leave alone as a change that happened without any discussion.

@handrews
Copy link
Contributor

@epoberezkin I'm with you on this

@handrews handrews mentioned this pull request Nov 16, 2016
@awwright
Copy link
Member

I think the idea is there's six primitive types, as far as parsing data is concerned; however in the "type" keyword you can also list "integer" which is a subset of "number" (any number with a zero fractional part).

@awwright
Copy link
Member

@kw217 Does #147 work?

@ramsey
Copy link

ramsey commented Nov 22, 2016

The removal of "integer" was a point of confusion for me. Can a line be added to the change log to note that "integer" was removed?

@epoberezkin
Copy link
Member

@ramsey integer was removed by mistake, #147 fixes it, this PR should be closed

@ramsey
Copy link

ramsey commented Nov 22, 2016

Thanks for the clarification.

@awwright
Copy link
Member

#147 describes the intended behavior so I'll close this out. If there's any further problems please feel free to open a new issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants