-
Notifications
You must be signed in to change notification settings - Fork 83
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
RFC: 204 - Go language bindings #206
Conversation
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
451ef0c
to
db13ef0
Compare
1453895
to
ae58e63
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.
Cool beans! Looking forward to this.
would need to generate an interface in addition to a concrete struct for each JSII class. | ||
|
||
The JSII [ClassType](https://github.com/aws/jsii/blob/master/packages/%40jsii/spec/lib/assembly.ts#L803) provides information on whether a | ||
class is abstract, whether it extends another class, and whether it implements other interfaces. We will discuss each case in turn. |
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'd like to see an example involving model protected members, and also polymorphic calls. Combining them into one example, how about:
class Animal {
public speak() {
console.log(this.voice());
}
protected voice() {
return '...';
}
}
class Poodle extends Animal {
protected voice() {
return 'arf';
}
}
// Expecting to see 'arf'
new Poodle().speak();
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.
Does the example below showing how move()
is overridden not fit the bill?
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.
Go may need to require "protected" members to be exported as public
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 think in jsii visibility is already required to be unchanged between inheriting classes (and hopefully releases, TO BE CHECKED), so we could do something like prepend a _
for protected members.
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.
Does the example below showing how
move()
is overridden not fit the bill?
I don't think so. What I'm looking for is a polymorphic self
call.
In all other forms of delegation you've shown, a child class is upcalling to its superclass--but that is fine, it knows the type of that superclass.
What if a superclass calls into a virtual method on itself, that may be implemented and overridden by a subclass? You'd need a reference to the current object's interface type, not its struct type.
I mean, I guess there's probably syntax for it, I just don't know what it is.
Previously, the CDK to generate the cloudformation templates lived elsewhere. Now they are inlined. In future, the aim is that Go itself can be used for these: aws/aws-cdk-rfcs#206 That's the hope, anyway!
f795fad
to
99a73c3
Compare
|
||
### Case 4: Abstract Classes | ||
|
||
These should be able to be handled much in the same way as regular classes, by generating an interface and a struct. The struct generation is still |
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 way to prevent direct instantiation of these structs to mimic the "abstract" part? Maybe a default New
that throws an error?
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.
Hm, perhaps but that wouldn't necessarily stop anyone from trying to declare the generated struct literal, right?
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.
Thank you!
text/204-golang-bindings.md
Outdated
export interface HealthCheck { | ||
readonly command: string[]; | ||
readonly interval?: cdk.Duration; | ||
readonly retries?: number; | ||
readonly startPeriod?: cdk.Duration; | ||
readonly timeout?: cdk.Duration; | ||
} | ||
|
||
function renderHealthCheck(hc: HealthCheck): cfntaskdefinition.HealthCheckProperty { | ||
return { | ||
command: getHealthCheckCommand(hc), | ||
interval: hc.interval != null ? hc.interval.toSeconds() : 30, | ||
retries: hc.retries !== undefined ? hc.retries : 3, | ||
startPeriod: hc.startPeriod && hc.startPeriod.toSeconds(), | ||
timeout: hc.timeout !== undefined ? hc.timeout.toSeconds() : 5, | ||
}; | ||
} | ||
``` | ||
|
||
Go translation (Run example in the [Go playground](https://play.golang.org/p/o84uoUGORYH): | ||
|
||
```go | ||
package ecs | ||
|
||
type IHealthCheck interface{ | ||
GetCommand() []string | ||
GetInterval() cdk.Duration; | ||
GetRetries() int | ||
GetStartPeriod() cdk.Duration; | ||
GetTimeout() cdk.Duration; | ||
} |
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 example is missing using pointer for the TS optional members. It may also be helpful to callout how optional properties, (and I assume function parameters) are translated into Go.
would need to generate an interface in addition to a concrete struct for each JSII class. | ||
|
||
The JSII [ClassType](https://github.com/aws/jsii/blob/master/packages/%40jsii/spec/lib/assembly.ts#L803) provides information on whether a | ||
class is abstract, whether it extends another class, and whether it implements other interfaces. We will discuss each case in turn. |
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.
Go may need to require "protected" members to be exported as public
@Mergifyio update |
Command
|
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.
💯
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license