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

Use SimpleDelegator for a delegation pattern #12842

Merged
merged 2 commits into from
Dec 3, 2019
Merged

Conversation

pkarman
Copy link
Contributor

@pkarman pkarman commented Dec 3, 2019

Small refactor to reduce code. The SimpleDelegator standard library allows for the delegation pattern in less code.

Also renames the test to match the tested class path.

@pkarman pkarman self-assigned this Dec 3, 2019
@codeclimate
Copy link

codeclimate bot commented Dec 3, 2019

Code Climate has analyzed commit 422d3d4 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Contributor

@nanotone nanotone left a comment

Choose a reason for hiding this comment

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

A couple very general questions (this PR looks fine):

  • SimpleDelegator, not being a module or concern, seems like it can't be used if you actually want to inherit from some other class for business logic, right?
  • It's also not possible to delegate methods to multiple destinations, or delegate only some method names?

@pkarman
Copy link
Contributor Author

pkarman commented Dec 3, 2019

@nanotone I would start with the docs: https://ruby-doc.org/stdlib-2.5.1/libdoc/delegate/rdoc/Delegator.html and https://ruby-doc.org/stdlib-2.5.1/libdoc/delegate/rdoc/SimpleDelegator.html

SimpleDelegator, not being a module or concern, seems like it can't be used if you actually want to inherit from some other class for business logic, right?

Inheritance and delegation are 2 different OO patterns for extending the functionality of existing classes. So it's not correct IMO to talk about them as mutually exclusive for business logic. They just have pros/cons in different contexts and can be used together.

You can use delegation to extend a class and implement business logic, as the class does in this PR.

It's also not possible to delegate methods to multiple destinations, or delegate only some method names?

If you wanted to delegate only some method names, then you would probably use the built-in Ruby delegate instead. Same pattern, different mechanism.

In this case, we were delegating so many methods on the RequestIssue object that it just made more sense to me to delegate all of them by default, since we weren't actively trying to not delegate some.

@pkarman pkarman added the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Dec 3, 2019
@va-bot va-bot merged commit 3a75164 into master Dec 3, 2019
@va-bot va-bot deleted the pek-workflow-delegator branch December 3, 2019 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants