Include base controller helpers when possible #1299
Conversation
waj
force-pushed
the
include-base-class-helpers
branch
from
8d1db6c
to
d3e1fb7
Compare
| sig { returns(T.untyped) } | ||
| def current_user_name; end | ||
|
|
||
| sig { params(user_id: ::Integer).void } |
There was a problem hiding this comment.
Removing the duplication is nice but I don't think it's worth losing the typing information in the signature. I believe depending on the loading order for Sorbet the signature in BaseController can take precedence.
There was a problem hiding this comment.
I'm not following, what typing information is lost? The helper methods are still properly defined in the base module with the type signature.
There was a problem hiding this comment.
Oh... I see what you mean. I didn't notice in this case the signature for notify_user was different in the base class. I'll see how can I avoid this case.
There was a problem hiding this comment.
Yeah now someone could write UserController.new.helpers.notify_user("Not an int") without any typechecking errors.
Motivation
In a Rails project is common to have helpers defined in a base controller. Currently those helpers are declared on every
.rbifile for descendant controllers unless those controllers define new helpers. The problem with this approach is that whenever a slight change is made on the base controller helpers, most.rbifiles for descendants get updated making reviews of PRs annoying.Implementation
If the helpers module of a controller is the same helpers module of it superclass then it's guaranteed that the helpers methods are the same. So, instead of generating the helpers methods, use the helper methods module generated for the parent class.
Tests
There was already a test with the case I described, so I updated the test to check that the subclass module just includes the parent's class module.