-
Notifications
You must be signed in to change notification settings - Fork 115
Add initial rest-api-spec converter #5295
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
base: main
Are you sure you want to change the base?
Conversation
Following you can find the validation changes against the target branch for the APIs.
You can validate these APIs yourself by using the |
* Add spec and docs for new logs streams endpoints * Add @codegen name to Acked responses * Linting fix * Update specification/streams/status/examples/200_response/GetStreamsStatusResponseExample1.yaml Co-authored-by: Copilot <[email protected]> * Correct feature flag * Add doc-ids * Fix doc-ids * Update specification/_doc_ids/table.csv Co-authored-by: Quentin Pradet <[email protected]> * Update specification/_doc_ids/table.csv Co-authored-by: Quentin Pradet <[email protected]> * Add default timeouts --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: Quentin Pradet <[email protected]>
Describes the `?bytes=` and `?time=` parameters which are accepted by all the `GET _cat/...` APIs.
Co-authored-by: pquentin <[email protected]>
Co-authored-by: pquentin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, left some minor comments.
if self.required { | ||
state.serialize_field("description", &self.description)?; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we skip the description if the body isn't required? From what I see in the json-spec, there's always a description
in body
even if not required.
Is this is the desired behavior, a simpler approach could be to use description: Option<String>
and set it at conversion time instead of implementing a custom serializer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I addressed this in #5295 (bce6bb60), which I wrote before your review. I'm sorry I didn't push it earlier. The reason the fix is that simple is that I enforced body.required
in the rest-api-spec last week:
|
||
let mut default = property.server_default.as_ref().map(|default| match default { | ||
clients_schema::ServerDefault::String(s) => serde_json::Value::String(s.clone()), | ||
clients_schema::ServerDefault::Number(n) => serde_json::Value::from(*n as i64), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n
is a f64
in clients_schema. Is the conversion to i64
intentional considering that it can be lossy?
return false; | ||
} | ||
|
||
fn is_literal(instance: &InstanceOf) -> Option<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: is_xxx
are generally used for boolean predicates. Rename to get_literal
?
Closes #5215