-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[docs] Fix generating constructor examples for resources that have numeric enums as input #16223
Conversation
Changelog[uncommitted] (2024-05-29)Bug Fixes
|
"fooNumericEnum": { | ||
TypeSpec: schema.TypeSpec{ | ||
Ref: "#/types/test:index:ExampleEnum", | ||
}, | ||
}, |
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.
Should we also add a test case for non-numeric enum?
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.
Done ✅
from.Type(), from, to)) | ||
} else { | ||
// for numeric enums, we new up the type that is generated for that enum | ||
convertFn = fmt.Sprintf("new %s.%s", pkg, name) |
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.
Is there a test that demonstrates this? Do we really use new
?
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 looked at the generated and their was a constructor that accepts double
but now I checked again and that constructor is private
😅
private TeamStackPermissionScope(double value)
there is actually no way to construct the enum from a number (it's generated as a struct). I changed the previous behaviour because it was panicking too early and unnecessarily so I though I would just fix it but this won't do. Will bring back the previous behaviour without redundantly panicking. Also we could change sdk-gen in dotnet to make the constructor public or add an implicit cast operator but that is for another PR.
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.
Old behaviour is back without panicking too early ✅ constructing a numeric enum from a number value will be handled in a separate PR
c8fbb92
to
a31a6db
Compare
5bc364b
to
0c9178d
Compare
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.
Mostly looks ok, odd that java and yaml are using the numeric values rather than enums though? Is that because they need fixes in their code generators?
@Frassle that's right |
090b6a4
to
b075f9d
Compare
Description
Fixes #16191
The original issue is that the intermediate PCL we generate used enum names instead of enum values for numeric enum inputs. This PR changes it so that the PCL program now uses the first numeric value for the first enum case then subsequently fixing downstream program-gen bugs that didn't know how to handle numeric values as inputs for enums.
Checklist
make tidy
to update any new dependenciesmake lint
to verify my code passes the lint checkgofumpt
make changelog
and committed thechangelog/pending/<file>
documenting my change