prefer class attribute over cattr_accessor in miq_policy #20790
Conversation
|
Checked commit d-m-u@080e97b with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint |
There was a problem hiding this comment.
So I’m guessing the @@ and cattr are breaking down due to the shared parent class but I’m not fully there. I’m typically pretty confident when it comes to classes/singleton classes and class variables so I’m in an uncomfortable and unfamiliar place. Could you share more info about the construct and why you feel we are not breaking something and what we hope to gain?
Thnx Drew
| @@ -51,7 +51,7 @@ class MiqPolicy < ApplicationRecord | |||
|
|
|||
| attr_accessor :reserved | |||
|
|
|||
| cattr_accessor :associations_to_get_policies | |||
| class_attribute :associations_to_get_policies | |||
|
|
|||
| @@built_in_policies = nil | |||
There was a problem hiding this comment.
Does this need to change too? This is a cattr.
| @@ -51,7 +51,7 @@ class MiqPolicy < ApplicationRecord | |||
|
|
|||
| attr_accessor :reserved | |||
|
|
|||
| cattr_accessor :associations_to_get_policies | |||
| class_attribute :associations_to_get_policies | |||
There was a problem hiding this comment.
The use of self To clarify the variable had me confused. Also the << not seeming to work but += did (for arrays). Could you explain the issue before and why we are not concerned with introducing bugs here?
|
thanks for the education. |
since cattr_accessors are potentially problematic re: wandering class vars, I think this should also probably be a class attribute
I'm not overly concerned with this one exhibiting the problematic behavior, however, because of the work done on #20274 this caught my eye because of Jason's #20664 (comment). as a general rule we probably shouldn't be using cattr_accessor.
see #20785, in which the same change was made, for the same reason, with the same level of concern