Skip to content

Fix method redefinition warnings #2137

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

Merged
merged 2 commits into from
Oct 17, 2019

Conversation

jonathanhefner
Copy link
Contributor

Seahorse::Client::Base.define_operation_methods generically defines methods for a client API. Later, generated client class code explicitly defines those methods. To prevent method redefinition warnings, make define_operation_methods define the generic methods in an anonymous module, which is then included into the client class.

Also, fix a conflict between an attr_accessor and an explicitly defined reader method.

Fixes #2070.


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

@jonathanhefner
Copy link
Contributor Author

Sidenote: I'm not sure how necessary define_operation_methods is. Tests in aws-sdk-core fail without it, but it seems like it is completely obviated by the generated client code. If it could be removed, that would marginally reduce memory consumption (from all the methods it defines, that are subsequently discarded). Perhaps someone with more insight can comment.

@mullermp
Copy link
Contributor

Thanks for opening this. Sorry for the delay (was out on vacation). When I first looked at this, I was thinking similarly. @cjyclaire has been on the team long enough to know the answer to that.

@mullermp mullermp added the pr/needs-review This PR needs a review from a Member. label Oct 16, 2019
@cjyclaire
Copy link
Contributor

Thanks for the contribution, and appreciate the thoughts there!

The reason for #set_api is that, there are many plugins/middleware stack registered per client are still using the @api definition to achieve functionalities, we are definitely thinking about removing this pattern and doing all static generation in the long run :) , but for the short term future, it's still needed.

Taking a look at the diff now

Copy link
Contributor

@cjyclaire cjyclaire left a comment

Choose a reason for hiding this comment

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

Looks great, would appreciate it if a changelog entry can be added here, thus we can track this in our next release :)

@mullermp mullermp added pr-in-progress and removed pr/needs-review This PR needs a review from a Member. labels Oct 17, 2019
@jonathanhefner jonathanhefner force-pushed the fix-method-redefinition-warnings branch from eea7333 to 1670627 Compare October 17, 2019 22:46
`Seahorse::Client::Base.define_operation_methods` generically defines
methods for a client API.  Later, generated client class code explicitly
defines those methods.  To prevent method redefinition warnings, make
`define_operation_methods` define the generic methods in an anonymous
module, which is then included into the client class.

Fixes aws#2070.
Use `attr_writer` instead of `attr_accessor` to prevent conflict with
explicitly defined reader method.
@jonathanhefner jonathanhefner force-pushed the fix-method-redefinition-warnings branch from 1670627 to b0a02b7 Compare October 17, 2019 22:51
@jonathanhefner
Copy link
Contributor Author

@cjyclaire Done!

@mullermp mullermp merged commit 2d1c862 into aws:master Oct 17, 2019
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.

Numerous method redefinitions in aws-sdk-core
3 participants