[SDQ-2009] lint missing super #17
Conversation
rhunterharris
requested review from
marvin-bitterlich,
mariodarco,
idperez,
ayazahmadtarar-pikessoft,
taimoorsendoso,
mcheemaa,
GS-Shahid and
pragathishankar
| Lint/MissingSuper: | ||
| Description: Checks for the presence of constructors and lifecycle callbacks without calls to ‘super`. | ||
| Enabled: true | ||
| Exclude: | ||
| - app/services/**/* | ||
|
|
There was a problem hiding this comment.
I think we should use AllowedParentClasses (if possible) instead of excluding full directories
https://docs.rubocop.org/rubocop/1.54/cops_lint.html#lintmissingsuper
There was a problem hiding this comment.
That would include:
ApplicationService
ValidateGiftCsvRow
BaseHandler
AbstractHandler
Sends:TrackerServices:Base
Do we anticipate other Rails microservices to implement these parent classes?
There was a problem hiding this comment.
Agreed with Marc. Unless a good reason, super should be used.
Its already linked, sharing for posterity:
rubocop/ruby-style-guide#809 (comment)
The problem is that you don't know this is the case if the superclass is part of another library or maintained by someone else. The base class may add some initialization logic in the future that would not get run by your subclass.
It is worth pointing out that the Java language has required all constructors to call a superconstructor since the beginning for exactly this reason: not calling the superclass constructor breaks the guarantee that it will run when instances of that class or its subclasses are instantiated. Java code requires that the subclass invoke a superclass constructor directly, or else a call to the superclass's default (no-args) constructor will be inserted for you.
There was a problem hiding this comment.
Sounds like we should keep this for full enforcement then, with no exceptions?
There was a problem hiding this comment.
(it also sounds like I should instead go ahead and start full rubocop enforcement per #14, so that it is no longer possible to bypass this)
There was a problem hiding this comment.
Sounds like we should keep this for full enforcement then, with no exceptions?
Ok with creating exceptions for the classes listed above (I'll double check these and share here)
There was a problem hiding this comment.
I guess, what I'm trying to articulate is that list of classes doesn't feel like the appropriate solution outside of the monolith. If we share this repo/gem among other classes, it should only contain contents that are relevant to all rails repos (like whitelisting a widely shared pattern like app/services). If we're including ApplicationService with something like CookieCutter, than it feels more appropriate to whitelist here. However the others should be whitelisted in the monolith, not in this repo.
Relatedly, if we have this sort of complicated set of rules, that points to a need to go ahead and start enforcing more strictness with our rules (which is an underlying sentiment I'm seeing here based on visible patterns + these comments)
There was a problem hiding this comment.
Agree – we could just override this in the application Rubocop config instead of in the gem then
Remove enforcement of Lint/MissingSuper for app/services directory and subdirectories, matching overrides in monolith.