-
-
Notifications
You must be signed in to change notification settings - Fork 719
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 rubocop Style/Send group #12703
Fix rubocop Style/Send group #12703
Conversation
@@ -754,7 +754,7 @@ | |||
end | |||
|
|||
it "assigns data to instance variables" do | |||
controller.send(:load_form_data) | |||
controller.__send__(:load_form_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just my two cents here:
- I believe the use of
__send__
method should be avoided as it breaks the encapsulation principle of OOP. Private methods are meant to be used within the classes they are defined in, and this method just invokes the methods that were not intended to be used outside of the class. - I prefer to use its public counterpart (
public_send
) where there is a need to call the methods dynamically. - This PR can be the best opportunity to identify these private method calls and make them either public or see a better alternative.
- What's your take on this, @openfoodfoundation/core-devs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - thanks for the review. There were similar thoughts which occurred to me as well - I had a quick look at a few of the methods in question the ones I looked at seemed to be relatively short and straightforward (but also made sense as private methods eg. load_form_data
which just sets a few instance variables for the controller class).
But really it made me wonder if these private methods need to be tested at all - as long as the public methods that call them are tested - but there's also an argument that it could make those tests more complex too. I'd be curious to hear others' opinions as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with what both of you are saying. We should really test the logic of these private methods via the public interface of the object. And I believe, it can reveal design issue in the code. We should remove these send
but this is not really the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @johansenja - Thank you so much for your contribution. You did great, 🎉
I just have a thing with send
as commented below. Just need a couple of core devs thoughts on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice ! thanks @johansenja 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tricky one.
I agree with others that wherever send
/ __send__
is used, it should be cause to question its use!
But as Gaetan suggests, we don't need to embark on that journey in this PR. I'm not sure we need to at all: this is legacy code and I don't think it's worth spending time refactoring just for the sake of it. I would definitely try to avoid using it any new specs.
In any case, this change removes a todo
, and, I think importantly, the __send__
name is much more conspicuous which makes it all the more obvious that it is a naughty method to be avoided!!
What? Why?
Fixed all the Style/Send offenses - with a simple find/replace across the affected files (only spec files), followed by autofixing any new issues which arose as a result (line length or alignment)
What should we test?
The affected files are only spec files, so the specs hopefully speak for themselves
Release notes
Changelog Category (reviewers may add a label for the release notes):