Support multiple superclasses #98
Conversation
| return unless (superclass = node.children[1]) | ||
|
|
||
| superclass.const_name == model_superclass_name | ||
| superclass = node.children[1] |
There was a problem hiding this comment.
Did you consider looking for all leaf nodes that include ApplicationRecord or ActiveRecord::Base in their hierarchy instead of just looking at immediate children?
There was a problem hiding this comment.
So unfortunately it's not this straightforward because this is not a Ruby object, it's an AST node. As far as we could see this only has the context of the current file, so you'd have to do a bunch of finnagling to be able to look at the class hierarchy outside of that context.
There was a problem hiding this comment.
+1 , happy to have another look into some ways around this if blocking though :~)
There was a problem hiding this comment.
I think it's probably fine. I guess we could potentially add a spec in payments-service to ensure all classes that inherit from ApplicationRecord and have children of their own are included in the ModelSuperclass list, but I think as @benk-gc says there's a limit to what we can check from a cop without a lot of extra work.
Maybe something that can be looked into another time. "Good enough" is better than not improving things because they're not yet perfect after all :)
| Lint/DefineDeletionStrategy: | ||
| ModelSuperclass: | ||
| - Acme::Record | ||
| - UmbrellaCorp::Record |
There was a problem hiding this comment.
I hope UmbrellaCorp get a high risk rating XD
| return unless (superclass = node.children[1]) | ||
|
|
||
| superclass.const_name == model_superclass_name | ||
| superclass = node.children[1] |
There was a problem hiding this comment.
I think it's probably fine. I guess we could potentially add a spec in payments-service to ensure all classes that inherit from ApplicationRecord and have children of their own are included in the ModelSuperclass list, but I think as @benk-gc says there's a limit to what we can check from a cop without a lot of extra work.
Maybe something that can be looked into another time. "Good enough" is better than not improving things because they're not yet perfect after all :)
Should have been done in #98, but I missed that it hadn't been
Should have been done in #98, but I missed that it hadn't been
Add support for linting detection for multiple model super classes without introducing breaking changes. Allow config to take either a list or a string for model super class. Update the
README.mdand specs to reflect this change.Also fix some miscellaneous unrelated linting/rubocop offenses being flagged in checks.