Use inventory object writers and readers instead of hash #13923
Conversation
|
@durandom @Fryguy so here is the nice object like interface. But since method missing is slow, I need to cache the methods. So it dynamically creates a new InventoryCollection class and caches there the readers/writers, per model. Would be nice to have a nice class name though. @agrare @durandom @Fryguy @kbrock also anybody knows how to get all writers of a model with a better way than |
kbrock
changed the title
| @@ -186,8 +186,12 @@ def lazy_find(manager_uuid, key: nil, default: nil) | |||
| ::ManagerRefresh::InventoryObjectLazy.new(self, manager_uuid, :key => key, :default => default) | |||
| end | |||
|
|
|||
| def inventory_object_class | |||
| @inventory_object_class ||= Class.new(::ManagerRefresh::InventoryObject) | |||
There was a problem hiding this comment.
does this need the Class.new part?
There was a problem hiding this comment.
Yeah, so it's only way I found, where we will end up with the right readers/writers on a InventoryObject class, according to model. Otherwise, they were all adding readers/writers into the one InventoryObject class, so it ended up with methods of all models.
Is there some concern wit Class.new? It should be garbage collected as any other object, right?
There was a problem hiding this comment.
I would just wish I could give it a nice Class name using this construct
There was a problem hiding this comment.
Thats what I mean with the proper inheritance in my main comment. This is a somewhat hacky and magical way of defining new classes on the fly
Make method like interface instead of hashes, we need to cache the writter and reader of the data, but then the InventoryObject class ends up with methods combined from many models. We could do a quick check with manually defined list of allowed attributes, otherwise the check will be done when saving, where rails will complain about undefined setter of the model. So having inaccurate InventoryObject.methods is a price for having quick assignment into hash, using methods.
Ansible use methods instead of hash accessor
Creating InventoryObject class per InventoryCollection, that way we can have specific readers/writers on InventoryObjectm per InventoryCollection, therefore per model.
Fix rubocop issues
Ladas
force-pushed
the
use_inventory_object_writers_and_readers_instead_of_hash
branch
from
85a72fb to
aa7ee6e
Compare
Ladas
changed the title
|
Checked commits Ladas/manageiq@1977cca~...aa7ee6e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@miq-bot remove_label wip |
|
@kbrock right, so same as in Rails, you can pick whether to use a hash or methods :-) |
There was a problem hiding this comment.
I'm a bit torn by this. While I appreciate all the magic of finding out what fields can be written to, I'm missing the documentation that the attributes would serve.
I'd rather have InventoryObject the base class and specialized subclasses, that explicitly define attributes. This way devolopers can look at the subclasses and see what attributes they must or can provide.
But this will not change the way attributes are accessed (via methods and or hash syntax) - so I'm good with this for now, but expect it to change down the road 😄
👍
|
Yea, I like explicit over implicit. It will server as documentation, and also won't blow out the class method cache. I also agree that it is good for now. |
Blocked:
only commits 67bb76c,
b078e28, 3571da1
are relevant, rest is dependency