-
Notifications
You must be signed in to change notification settings - Fork 23
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
remove confusing type Enum or String
#215
Comments
@davidB we added the The initial approach was to only have an |
If the purpose is to document, why not use the field https://json-schema.org/understanding-json-schema/reference/annotations You should not only consider those schemas as a way to "validate" (in this case there is no validation), but also as definitions used to:
What it will look like in an API? A |
@davidB does this definition affect some tooling or something you are using? Im trying to understand what the exact problem is other than it can be more efficiently written. And it isn't just documenting. It's specifically saying, this is a string, and here are the list of possible values. The latter point is very important. If we just say string, then that means anything, but we want to limit that. If there is tooling that is being used that is having trouble with this definition, can we get some more information on that? That may help us figure out a way to approach the problem. |
@xibz, Yes this definition affect tooling (currently I customize it to say String only). Try to answer the question:
With a tool like https://app.quicktype.io/, it becomes a simple String, but if I do word to word manual translation of this definition in rust it should be something like: Enum Priority { // anyOf
PriorityEnum(PriorityEnum),
String(String),
}
Enum PriorityEnum {
Low,
Medium,
High,
} But on parsing, deserialization PriorityEnum will never be used, because it requires custom code to say if the string value in ... then use enum, else use string. (My issue is the need of this custom code for each type/field) And for user it also complexity the usage. json enum are for exhausted list. |
After playing around some with the JSON schema, I believe |
IMO, To be clear what will work for me (and for tool and reader) is something like: "priority": {
"description": "An indicator of the importance of the ticket",
"examples": [
"low",
"medium",
"high"
],
"type": "string"
} |
I think "anyOf": [
{
"type": "string",
"enum": [
"low",
"medium",
"high"
]
},
{
"type": "string" <- This is not needed
}
] I will go ahead and mark this as a bug, and we can fix it to look like, "oneOf": [
{
"type": "string",
"enum": [
"low",
"medium",
"high"
]
}
] Would that work for you? I think this will solve all the issues you are seeing at least with the tool |
in this case why using a |
@davidB Because a string'd enum is different than an normal enum, e.g. C where it is an integer, and we also want to explicitly say it is a string, and not anything else. It's mostly for clarity. |
in json-schema
and not
Sorry I don't understand the value of |
@davidB No, they don't have to be strings. We are explicitly saying that they are strings. This is important because anyone wanting to make a change to the spec, will know that it is explicitly as a string. "oneOf": [
{
"enum": [
1, <- this is valid
"low",
"medium",
"high"
]
}
] |
My question is why using a "oneOf" with an array of 1 value (the enum)? |
@davidB Oh, sorry, I misunderstood what you are asking. It's not an array in practice. That's just part of the spec. Basically it is saying, we have some I hope that makes sense? |
|
Oh! I completely mistook what you were saying! Gotcha! hahah, now I see the confusion. Okay, yea, this is definitely not modeled correctly. Okay, I will have this fixed with "enum": [
"low",
"medium",
"high"
] |
if |
Okay, I think I understand what was trying to be modeled. So basically we want any string, but want to provide some defaults, like Just providing |
Interesting - is |
and used by some OpenAPI and contract-testing tool like schemathesis to validate API or generate samples or validate API |
@davidB Examples are nice, but I think it's really important to have the values modeled in the SDKs. I believe |
@xibz Can you provide an example (in a programming language) about how you think those values could be modeled in the SDKs? |
type Priority string
var Low Priority = "low"
var Medium Priority = "medium"
var High Priority = "high" Something like this would be sufficient Then if a user wanted a custom value they could do something like fmt.Println(cdevents.Priority("my-custom-value"))
// or use a predefined value we've specified to help consistency
fmt.Println(cdevents.Low) |
@xibz We discussed this during the working group today and came to this proposal, let us know what you think:
"priority": {
"description": "An indicator of the importance of the ticket",
"examples": [
"low",
"medium",
"high"
],
"type": "string"
}
type Priority string
var Low Priority = "low"
var Medium Priority = "medium"
var High Priority = "high" |
@afrittoli I think that makes sense except for generating based on
While we could generate from there, other enums from examples may cause issues if we do not plan to have them as generated. So it feels hacky. Im personally fine with the current definition, as it is valid. However, if there is a more correct way to write it, I'd be for it. I don't know if |
Different modelling languages might allows us to express this better, so that's something we should look into. |
With version 0.4, ticket's schema define type of some field (
priority
,resolution
,ticketType
) like:But this definition is "conflicting" as the second is a superset of the first. (if
oneOf
is used instead ofanyOf
a jsonschema linter will warm about this issue). I mean for any language implementationstring
will be used (or an automatic generator/serializer+deserializer will be confused).What is the purpose/value of the enum if any value can be used? (eg if a system is uppercase like "LOW")
Suggestion: replace the
anyOf
by single"type": "string"
@sbrennan4 WDYT?
The text was updated successfully, but these errors were encountered: