Conversation
Summary of ChangesHello @kvaps, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the OpenAPI schema generation by introducing a mechanism to explicitly order parameters. By adding an Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThese changes introduce per-field ordering for OpenAPI Node elements through a new Order field and orderCounter mechanism. The WriteValuesSchema function is refactored to accept an optional root parameter, enabling ordered emission of parameter fields based on explicit Order values or lexical fallback. Integration updated in main.go to pass the tree parameter. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as main.go
participant Gen as gen<br/>(OpenAPI)
participant Node as Node<br/>(Tree)
participant Schema as Output<br/>Schema
Main->>Gen: WriteValuesSchemaWithOrder(crdBytes, outPath, tree)
Gen->>Node: Traverse tree, read Order fields
Node-->>Gen: Return nodes with Order values
Gen->>Gen: sortedKeysByOrder(nodes)<br/>Sort by Order or lexical
Gen->>Schema: Emit struct fields<br/>in determined order
Gen->>Schema: Emit values schema<br/>respecting Order
Schema-->>Main: JSON schema written
Main->>Main: Log: "write JSON schema: %s"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable sequence diagrams in the walkthrough.Disable the |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces ordering for parameters, which is then used to generate an ordered values.schema.json. The implementation adds an Order field to the Node struct and populates it as parameters are parsed. My review identified a critical bug in the sorting logic that fails to correctly order parameters and a medium-severity issue with JSON formatting that results in misaligned output. I've provided code suggestions to address both of these issues.
| sort.Slice(keys, func(i, j int) bool { | ||
| nodeI := nodes[keys[i]] | ||
| nodeJ := nodes[keys[j]] | ||
| if nodeI.Order != 0 || nodeJ.Order != 0 { | ||
| return nodeI.Order < nodeJ.Order | ||
| } | ||
| return keys[i] < keys[j] | ||
| }) |
There was a problem hiding this comment.
The sorting logic in sortedKeysByOrder is flawed and doesn't correctly order parameters.
- The first parameter is assigned
Order = 0, which is the same as the default for non-parameter fields. When comparing this parameter with a non-parameter, theif nodeI.Order != 0 || nodeJ.Order != 0condition is false, causing it to fall back to alphabetical sorting. This is incorrect. - When comparing a parameter with
Order > 0against a non-parameter withOrder = 0, the conditionnodeI.Order < nodeJ.Orderwill be false, causing the non-parameter to be sorted first.
A more robust approach is to use the IsParam boolean field, which clearly distinguishes parameters from other fields. This ensures parameters are always sorted before non-parameters, and then by their specified order.
sort.Slice(keys, func(i, j int) bool {
nodeI := nodes[keys[i]]
nodeJ := nodes[keys[j]]
if nodeI.IsParam && nodeJ.IsParam {
return nodeI.Order < nodeJ.Order
}
if nodeI.IsParam {
return true
}
if nodeJ.IsParam {
return false
}
return keys[i] < keys[j]
})| propJSON, err := json.MarshalIndent(prop, " ", " ") | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| buf.WriteString(fmt.Sprintf(" \"%s\": %s", key, string(propJSON))) |
There was a problem hiding this comment.
The manual JSON generation for properties results in incorrect indentation in the output values.schema.json file. Using json.MarshalIndent with a hardcoded prefix for the property value causes misalignment because the prefix is applied to every line of the marshaled object, including the first one.
To produce a correctly formatted JSON, you can marshal the property without a prefix and then manually handle the indentation to align it properly under its key.
propJSON, err := json.MarshalIndent(prop, "", " ")
if err != nil {
return err
}
// Manually indent the property JSON to align correctly.
lines := strings.Split(string(propJSON), "\n")
buf.WriteString(fmt.Sprintf(" \"%s\": %s", key, lines[0]))
for _, line := range lines[1:] {
buf.WriteString("\n " + line)
}
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
1 participant
Signed-off-by: Andrei Kvapil kvapss@gmail.com
Summary by CodeRabbit
New Features
Refactor