Skip to content
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

Merged
merged 29 commits into from
Dec 8, 2020
Merged

RFC: 204 - Go language bindings #206

merged 29 commits into from
Dec 8, 2020

Conversation

SoManyHs
Copy link
Contributor

@SoManyHs SoManyHs commented Jul 17, 2020


By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license

@mergify
Copy link
Contributor

mergify bot commented Jul 17, 2020

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@SoManyHs SoManyHs changed the title RFC 204: Go language bindings RFC: 204 - Go language bindings Jul 17, 2020
@SoManyHs SoManyHs force-pushed the master branch 5 times, most recently from 451ef0c to db13ef0 Compare July 18, 2020 00:15
eladb
eladb previously requested changes Jul 19, 2020
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
@mergify mergify bot dismissed eladb’s stale review July 20, 2020 17:20

Pull request has been modified.

@SoManyHs SoManyHs force-pushed the master branch 3 times, most recently from 1453895 to ae58e63 Compare July 20, 2020 22:34
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
Copy link
Contributor

@rix0rrr rix0rrr left a 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.

text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Show resolved Hide resolved
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.
Copy link
Contributor

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();

Copy link
Contributor Author

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?

Copy link

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

Copy link
Contributor

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.

Copy link
Contributor

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.

text/204-golang-bindings.md Outdated Show resolved Hide resolved
nicl added a commit to guardian/nest that referenced this pull request Aug 4, 2020
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!
@SoManyHs SoManyHs force-pushed the master branch 3 times, most recently from f795fad to 99a73c3 Compare August 11, 2020 02:38
text/204-golang-bindings.md Outdated Show resolved Hide resolved

### 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
Copy link
Contributor

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?

Copy link
Contributor Author

@SoManyHs SoManyHs Dec 3, 2020

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?

Copy link

@kohidave kohidave left a 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 Show resolved Hide resolved
text/204-golang-bindings.md Show resolved Hide resolved
Comment on lines 186 to 216
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;
}
Copy link

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.

text/204-golang-bindings.md Outdated Show resolved Hide resolved
text/204-golang-bindings.md Outdated Show resolved Hide resolved
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.
Copy link

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

@MrArnoldPalmer
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Dec 7, 2020

Command update: success

Branch has been successfully updated

Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants