-
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
Go and Python codegen support symbols with hyphens in their names #10738
Conversation
Please view the results of the Downstream Codegen Tests Here |
Changelog[uncommitted] (2022-10-10) |
09505d3
to
f3d4f46
Compare
Please view the results of the Downstream Codegen Tests Here |
22fbc1b
to
0a05b53
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.
This looks good to me.
@mattolenik @iwahbe Should we modify some programgen test cases to cover changes from this PR? |
return string(res) | ||
} | ||
|
||
// UppercaseFirst uppercases the first letter of s. |
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.
This is called Title elsewhere
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.
True, though there are package-specific implementations of Title
that can (possibly) contain different logic. They get used like this:
// From pkg/codegen/docs/utils.go
func title(s, lang string) string {
switch lang {
case "go":
return go_gen.Title(s)
case "csharp":
return dotnet.Title(s)
default:
return strings.Title(s)
}
}
And since Title
has the association (at least to me) of being like strings.Title
which will uppercase multiple letters and not just the first one. Like foo bar
becomes Foo Bar
not Foo bar
which is what we want here. So I figured a new name was clearer, but happy to take suggestions anyway of course
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.
That's fair enough reasoning to leave it as is
Or add new ones |
42fa179
to
ada2764
Compare
@iwahbe @AaronFriel should we take this PR over and bring it to the finish line? |
Previously codegen would output hyphens into the names of symbols such as functions, variable names, etc. This fix converts hyphens to underscores, making them valid code in Go and Python.
Yes we should. I'm on it. |
Convert to draft & remove reviewers until ready for review? |
It should be ready for review now. @mattolenik addressed most of the feedback he got. I cleaned up the rest and regenerated the test case. It can be reviewed now. |
bors r+ |
Build succeeded: |
11033: Revert "Go and Python codegen support symbols with hyphens in their names" r=iwahbe a=iwahbe Reverts #10738 This caused changes in python class names. Co-authored-by: Ian Wahbe <[email protected]>
11033: Revert "Go and Python codegen support symbols with hyphens in their names" r=iwahbe a=iwahbe Reverts #10738 This caused changes in python class names. Co-authored-by: Ian Wahbe <[email protected]>
Description
This change enables the fix for pulumi/crd2pulumi#43, wherein hyphens
-
were mishandled and led to generation of invalid Go and Python. For example, code such asfunc My-Thing() {}
orclass My-Thing
.This change improves the existing casing functions and extracts them to a new subpackage,
cgstrings
.Resolves pulumi/crd2pulumi#43. When this change is merged
go get -u
will fix the crd2pulumi issue.Checklist