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

fix(go): fix generation of readonly and static properties #2133

Merged
merged 2 commits into from
Oct 21, 2020

Conversation

SoManyHs
Copy link
Contributor

@SoManyHs SoManyHs commented Oct 15, 2020

Previously, we were generating setters for readonly properties, as well as including static properties as part of a class interface and implementation. This fixes the property generation to omit the setter declarations/implementations for readonly properties. It also generates static properties as the package level.

Fixes #2093


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

@SoManyHs SoManyHs added the language/go Regarding GoLang bindings label Oct 15, 2020
@SoManyHs SoManyHs added this to the GoLang Alpha milestone Oct 15, 2020
@SoManyHs
Copy link
Contributor Author

SoManyHs commented Oct 15, 2020

NOTE: It maybe worth considering refactoring this so that the static properties are wrapped in a StaticProperty class and then have a list StaticProperty[] as a member on the GoClass as we do with staticMethods; however, I wanted to wait until the StaticGet jsii call is implemented before doing so. I tried a few ways of having a StaticProperty class extend from existing classes, but none of the abstractions seemed to quite fit so simply made a emitStaticProperty private method on the class instead of following the usual SomeConstruct.emit() pattern. @MrArnoldPalmer

@SoManyHs SoManyHs mentioned this pull request Oct 15, 2020
4 tasks
@SoManyHs SoManyHs force-pushed the fix-props branch 3 times, most recently from 60ff599 to 6d889b3 Compare October 15, 2020 23:12
@MrArnoldPalmer
Copy link
Contributor

NOTE: It maybe worth considering refactoring this so that the static properties are wrapped in a StaticProperty class and then have a list StaticProperty[] as a member on the GoClass as we do with staticMethods; however, I wanted to wait until the StaticGet jsii call is implemented before doing so. I tried a few ways of having a StaticProperty class extend from existing classes, but none of the abstractions seemed to quite fit so simply made a emitStaticProperty private method on the class instead of following the usual SomeConstruct.emit() pattern. @MrArnoldPalmer

I think this is the right way to go. That would also remove the need for these checks

   for (const prop of this.properties) {
      if (prop.static) {
        // emit static get
...
    for (const property of this.properties) {
      if (!property.static) {
        // emit non-static get
...

There isn't a lot of shared behavior between static and non-static property getters so a common ancestor wouldn't have much. Basically the only common thing it needs is the stuff declared on the GoTypeMember + the immutable property.

This could warrant an abstract wrapper class, or maybe just another interface like

interface GoPropertyMember extends GoTypeMember {
  immutable: boolean
}

Since they would take the same constructor args maybe an abstract class is best.

RomainMuller
RomainMuller previously approved these changes Oct 19, 2020
@RomainMuller RomainMuller added the pr/do-not-merge This PR should not be merged at this time. label Oct 19, 2020
@RomainMuller
Copy link
Contributor

Added pr/do-not-merge if you wanna take the refactor route in this PR (I'm cool if you prefer to do it separately, too - in which case just drop the label or have @MrArnoldPalmer drop it).

@SoManyHs SoManyHs removed the pr/do-not-merge This PR should not be merged at this time. label Oct 20, 2020
@mergify mergify bot dismissed RomainMuller’s stale review October 20, 2020 00:05

Pull request has been modified.

RomainMuller
RomainMuller previously approved these changes Oct 20, 2020
@SoManyHs
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Oct 20, 2020

Command update: success

Branch already up to date

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
  • Commit ID: 9560eb1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@SoManyHs
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Oct 21, 2020

Command update: success

Branch already up to date

@RomainMuller RomainMuller removed the request for review from MrArnoldPalmer October 21, 2020 08:31
@mergify
Copy link
Contributor

mergify bot commented Oct 21, 2020

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Oct 21, 2020
@RomainMuller
Copy link
Contributor

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Oct 21, 2020

Command refresh: success

Pull request refreshed

@mergify mergify bot merged commit 57b7d56 into aws:master Oct 21, 2020
@mergify
Copy link
Contributor

mergify bot commented Oct 21, 2020

Merging (with squash)...

@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language/go Regarding GoLang bindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Go properties handle "readonly" modifier
4 participants