| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Greptile SummaryThis PR adds optional named type generation for inline schemas. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "codegen: warn when generate-types-for-an..." | Re-trigger Greptile |
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
Functionally it looks good - I wonder if we can improve the generated Description that gets added to it?
It's generally an existing issue, but especially where we're generating this new anonymous schema, it'd be great to improve
I..e we get:
// AddMicrosoftTeamsChatTabTaskParams defines model for add_microsoft_teams_chat_tab_task_params.
type AddMicrosoftTeamsChatTabTaskParams struct {
- Chat struct {
- ID *string `json:"id,omitempty"`
- Name *string `json:"name,omitempty"`
- } `json:"chat"`
+ Chat AddMicrosoftTeamsChatTabTaskParams_Chat `json:"chat"`
// Link The tab link
Link string `json:"link"`
@@ -39399,15 +39402,18 @@ type AddMicrosoftTeamsChatTabTaskParams struct {
Title string `json:"title"`
}
+// AddMicrosoftTeamsChatTabTaskParams_Chat defines model for AddMicrosoftTeamsChatTabTaskParams.Chat.
+type AddMicrosoftTeamsChatTabTaskParams_Chat struct {
+ ID *string `json:"id,omitempty"`
+ Name *string `json:"name,omitempty"`
+}I wonder if we could instead generate it as i.e.
// AddMicrosoftTeamsChatTabTaskParams_Chat defines model for the anonymous schema `chat` under path ..., or something similar?
Sorry, something went wrong.
Ok, so the code which generates those comments is kinda broken. It calls everything a "parameter". I'm going to make a minimal fix to this PR to inject the correct string with the field name, eg, AddMicrosoftTeamsChatTabTaskParams.Chat., however, the rest of the string will remain wrong. I'll file an Issue for the misleading comments and deal with the high level problems separately. Update: |
Sorry, something went wrong.
Adds output-options.generate-types-for-anonymous-schemas (default false). When set, every inline schema that would otherwise generate as an anonymous Go struct is instead emitted as a named type with a path- derived name (e.g. GetRolesId200JSONResponseBody_Data). Equivalent to adding x-go-type-name to every inline schema; when both are present at the same site, x-go-type-name wins. Test coverage reorganized: internal/test/anonymous_inner_hoisting/ now has two subdirectories. The existing always-on hoisting test (oneOf, anyOf, additionalProperties) moves to implicit/. The new flag-gated behavior gets its own test in global/, exercising the canonical issue shape (allOf-merged response with a sibling inline `data` object that itself contains a nested `role` reference). Closes: oapi-codegen#1139
|
Greptile had a good finding. This refactor breaks the splitting of code generation into (types, server, strict-server), etc, because of references among the types. Operation Definitions can introduce new types (the anonymous inner types), but those are only emitted when types are being emitted, so if I generate types without a server, then say, server: echo into another file, then we will have missing types. the fix is non-trivial. It's a big refactor of the codegen structure. |
Sorry, something went wrong.
…s this config does not emit
The new `output-options.generate-types-for-anonymous-schemas` flag (issue
flow through the same emission path that `generate.models` gates. In a
single-config setup where the flag is set with `models: false` but a
client or server generator is enabled, the generated code emits
references to the hoisted names without declaring them, and `go build`
fails.
Multi-config setups where one config emits `models: true` and a sibling
emits a client/server into the same Go package work correctly today;
the hoisting decisions are deterministic across the two codegen
invocations and the sibling's declarations resolve same-package
references. The failure mode is specifically the single-config case.
The two cases are indistinguishable from a single codegen invocation
(both look like `flag: true, models: false, <ops>: true`), so this
warns rather than errors. Split-config users see an informational
notice they can ignore; misconfigured single-config users get a
heads-up before `go build` complains.
Changes:
- pkg/codegen/configuration.go:
- Add `GenerateOptions.AnyOperationGenerator()` reporting whether any
client/server/strict generator is enabled.
- Add `Configuration.Warnings()`, a cross-field diagnostic that fires
when `GenerateTypesForAnonymousSchemas && !Models &&
AnyOperationGenerator()`. Follows the existing
`GenerateOptions.Warnings()` pattern used for the std-http-server
Go-version check.
- Strengthen the doc comment on `GenerateTypesForAnonymousSchemas` to
call out the single-config failure mode and the multi-config
consistency requirement explicitly.
- configuration-schema.json: matching description text for the
IDE-surfaced schema doc.
- cmd/oapi-codegen/oapi-codegen.go: wire `opts.Warnings()` to stderr
immediately after the existing `Generate.Warnings()` printout, under a
"cross-field configuration warning(s)" header.
Helper-function cleanup in pkg/codegen/codegen.go:
- Decompose the monolithic `GenerateTypeDefinitions` into
`collectComponentTypes` (gathers #/components-derived TypeDefinitions),
`collectOperationTypes` (gathers op-derived TypeDefinitions for the
boilerplate scan), and `renderBoilerplate` (runs enum / additional-
properties / union / union+additional passes over the union). The
emission gating is unchanged — still a single `if opts.Generate.Models`
block — so this is a pure refactor with no behavioral effect on any
existing config. Generated output is byte-identical.
No new fixtures or behavioral changes; this commit is intended to be
squashed with the parent `Add optional type generation for inline
schemas` commit before the PR lands.
Resolved one conflict in pkg/codegen/codegen.go: upstream now uses the collectComponentTypes / collectOperationTypes / renderBoilerplate helper decomposition (from PR #2366). Kept that structure and changed the op- related helper calls to use allOps (paths + webhooks + callbacks) rather than ops, matching what the previous GenerateTypeDefinitions(t, spec, allOps, ...) call did, so webhook/callback-derived types continue to be emitted.
* Add optional type generation for inline schemas Adds output-options.generate-types-for-anonymous-schemas (default false). When set, every inline schema that would otherwise generate as an anonymous Go struct is instead emitted as a named type with a path- derived name (e.g. GetRolesId200JSONResponseBody_Data). Equivalent to adding x-go-type-name to every inline schema; when both are present at the same site, x-go-type-name wins. Test coverage reorganized: internal/test/anonymous_inner_hoisting/ now has two subdirectories. The existing always-on hoisting test (oneOf, anyOf, additionalProperties) moves to implicit/. The new flag-gated behavior gets its own test in global/, exercising the canonical issue shape (allOf-merged response with a sibling inline `data` object that itself contains a nested `role` reference). Closes: oapi-codegen#1139 * codegen: warn when generate-types-for-anonymous-schemas requires types this config does not emit The new `output-options.generate-types-for-anonymous-schemas` flag (issue flow through the same emission path that `generate.models` gates. In a single-config setup where the flag is set with `models: false` but a client or server generator is enabled, the generated code emits references to the hoisted names without declaring them, and `go build` fails. Multi-config setups where one config emits `models: true` and a sibling emits a client/server into the same Go package work correctly today; the hoisting decisions are deterministic across the two codegen invocations and the sibling's declarations resolve same-package references. The failure mode is specifically the single-config case. The two cases are indistinguishable from a single codegen invocation (both look like `flag: true, models: false, <ops>: true`), so this warns rather than errors. Split-config users see an informational notice they can ignore; misconfigured single-config users get a heads-up before `go build` complains. Changes: - pkg/codegen/configuration.go: - Add `GenerateOptions.AnyOperationGenerator()` reporting whether any client/server/strict generator is enabled. - Add `Configuration.Warnings()`, a cross-field diagnostic that fires when `GenerateTypesForAnonymousSchemas && !Models && AnyOperationGenerator()`. Follows the existing `GenerateOptions.Warnings()` pattern used for the std-http-server Go-version check. - Strengthen the doc comment on `GenerateTypesForAnonymousSchemas` to call out the single-config failure mode and the multi-config consistency requirement explicitly. - configuration-schema.json: matching description text for the IDE-surfaced schema doc. - cmd/oapi-codegen/oapi-codegen.go: wire `opts.Warnings()` to stderr immediately after the existing `Generate.Warnings()` printout, under a "cross-field configuration warning(s)" header. Helper-function cleanup in pkg/codegen/codegen.go: - Decompose the monolithic `GenerateTypeDefinitions` into `collectComponentTypes` (gathers #/components-derived TypeDefinitions), `collectOperationTypes` (gathers op-derived TypeDefinitions for the boilerplate scan), and `renderBoilerplate` (runs enum / additional- properties / union / union+additional passes over the union). The emission gating is unchanged — still a single `if opts.Generate.Models` block — so this is a pure refactor with no behavioral effect on any existing config. Generated output is byte-identical. No new fixtures or behavioral changes; this commit is intended to be squashed with the parent `Add optional type generation for inline schemas` commit before the PR lands.
|
In rootlyhq/rootly-go#131 I've noticed that some nullable entries are missing - might be intentional, but I'm taking a look |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Adds output-options.generate-types-for-anonymous-schemas (default false). When set, every inline schema that would otherwise generate as an anonymous Go struct is instead emitted as a named type with a path- derived name (e.g. GetRolesId200JSONResponseBody_Data). Equivalent to adding x-go-type-name to every inline schema; when both are present at the same site, x-go-type-name wins.
I will need to document this, since it's still better, from a usability perspective, to put concrete types of interest in component/schemas or as a second alternative, use x-go-type-name. However, this is requested often enough, that I'd thing it's worth exposing as a feature.
Test coverage reorganized: internal/test/anonymous_inner_hoisting/ now has two subdirectories. The existing always-on hoisting test (oneOf, anyOf, additionalProperties) moves to implicit/. The new flag-gated behavior gets its own test in global/, exercising the canonical issue shape (allOf-merged response with a sibling inline data object that itself contains a nested role reference).
Closes: #1139