-
Notifications
You must be signed in to change notification settings - Fork 0
Implement ordering for parameters #15
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
Signed-off-by: Andrei Kvapil <[email protected]>
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
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 [email protected]