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.
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.
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.
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.
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.
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):