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

feat(jsii-go): use reflect to resolve overriden methods #2780

Merged
merged 9 commits into from
Apr 9, 2021

Conversation

yglcode
Copy link
Contributor

@yglcode yglcode commented Apr 8, 2021

Use reflect to resolve overridden methods, per issue #2768, leveraging
how such methods will no longer be visible on the non-pointer receiver,
while methods promoted from anonymous embeds are.


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 Apr 8, 2021

The title of this Pull Request does not conform with [Conventional Commits] guidelines. It will need to be adjusted before the PR can be merged.
[Conventional Commits]: https://www.conventionalcommits.org

@yglcode yglcode changed the title use reflect to resolve overriden methods, per issue #2768 feat(jsii-go): use reflect to resolve overriden methods Apr 8, 2021
//if overriding struct has no overriding methods, could happen if
//overriding methods are not defined with pointer receiver.
if len(mOverrides) == 0 && !strings.HasPrefix(instType.Name(), "jsiiProxy_") {
panic(fmt.Errorf("%s has no overriding methods. Please verify overriding methods are defined with pointer receiver", instType.Name()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
panic(fmt.Errorf("%s has no overriding methods. Please verify overriding methods are defined with pointer receiver", instType.Name()))
panic(fmt.Errorf("%s has no overriding methods. Overriding methods must be defined with a pointer receiver", instType.Name()))

@mergify
Copy link
Contributor

mergify bot commented Apr 8, 2021

The title of this Pull Request does not conform with [Conventional Commits] guidelines. It will need to be adjusted before the PR can be merged.
[Conventional Commits]: https://www.conventionalcommits.org

@RomainMuller
Copy link
Contributor

Made some (vey) minor style changes in your code (mostly adding white spaces and capitalisation in comments)...

I wonder if the error message you added could be improved a little (but I'm not too concerned about it though).

That looks pretty good to me at this stage; it's a very clever idea and I like it.


Would you be able to update the documentation under gh-pages? I can take that part up otherwise (no problem!)

@yglcode
Copy link
Contributor Author

yglcode commented Apr 8, 2021

Thanks for your comments. I will update the code. It will be great if you could update the docs. Thanks.

Just updated the code.

@RomainMuller
Copy link
Contributor

Updated the docs page now to remove references to the overrides tag and insist on the fact overridden methods must have a pointer receiver... I think we're good to ship now...

Thank you again for the clever idea & implementation!

RomainMuller
RomainMuller previously approved these changes Apr 9, 2021
@mergify mergify bot dismissed RomainMuller’s stale review April 9, 2021 09:52

Pull request has been modified.

RomainMuller
RomainMuller previously approved these changes Apr 9, 2021
@mergify
Copy link
Contributor

mergify bot commented Apr 9, 2021

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 Apr 9, 2021
@mergify
Copy link
Contributor

mergify bot commented Apr 9, 2021

Merging (with squash)...

@mergify mergify bot dismissed RomainMuller’s stale review April 9, 2021 14:50

Pull request has been modified.

@RomainMuller
Copy link
Contributor

@yglcode - don't mind me I'm trying to diagnose the transient windows PR validation failure as it appears to be happening on this particular PR (no idea why or what's wrong 🤷🏻‍♂️).

I'll get your code merged soon (even if I don't manage to understand the failure).

@mergify mergify bot merged commit 295d189 into aws:main Apr 9, 2021
@mergify
Copy link
Contributor

mergify bot commented Apr 9, 2021

Merging (with squash)...

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

Successfully merging this pull request may close these issues.

2 participants