RDSK-2417 Remove ResourceType
#184
Conversation
| @@ -17,6 +17,7 @@ class MySummation : public Summation { | |||
| }; | |||
| void reconfigure(Dependencies deps, ResourceConfig cfg) override; | |||
| static std::vector<std::string> validate(ResourceConfig cfg); | |||
| void stop(const AttributeMap& extra) override{}; | |||
There was a problem hiding this comment.
There were quite a few places I had to add stop due to it being a purely virtual method (I don't really understand how #182 was merged without these changes). I would say I'm against having every resource implement its own stop method, and would rather use a noop default method for the time being. IMO there should be an ActuatingComponent class or something that inherits from Component with virtual is_moving and stop methods, but that might be work for another ticket? Let me know if I'm missing some reason for the purely virtual stop.
There was a problem hiding this comment.
There's a ticket to address this here, I think the current proposal is to have a StoppableResource class that can be inherited from.
I understand the inconvenience of having to implement a no-op stop for sure but I'm not convinced that having stop default is better. It's (trivially) less work and a bit less pointy, but it also makes it possible for people to potentially do Quite Bad things by failing to implement stop when it is necessary. My preference would be to back out the stop changes here and just focus on the ResourceType removal, and then make the more principled change in a separate PR.
There was a problem hiding this comment.
My bad; totally missed that ticket! Totally understand the concern; I'll remove all stop changes from this PR for now, thanks for the context.
benjirewis
force-pushed
the
remove-resource-type
branch
from
5fe24db to
b966d75
Compare
benjirewis
changed the title
ResourceType and add missing stop methodsResourceType and make stop default to a noop
benjirewis
force-pushed
the
remove-resource-type
branch
from
b966d75 to
6c0f500
Compare
benjirewis
changed the title
ResourceType and make stop default to a noopResourceType
Sorry I don't see any GH comments in the
I think it will break basically all C++ modules in the registry. |
| std::string resource_subtype; | ||
| std::vector<ResourceName> resource_names; | ||
| for (auto& kv : Registry::registered_models()) { | ||
| const std::shared_ptr<ModelRegistration> reg = kv.second; | ||
| if (reg->api().to_string() == resource->dynamic_api().to_string()) { | ||
| resource_type = reg->api().resource_subtype(); | ||
| resource_type = reg->api().resource_type(); | ||
| resource_subtype = reg->api().resource_subtype(); | ||
| } else { | ||
| continue; | ||
| } | ||
|
|
||
| if (resource_type.empty()) { | ||
| resource_type = resource->name(); | ||
| if (resource_subtype.empty()) { | ||
| resource_subtype = resource->name(); | ||
| } | ||
|
|
||
| ResourceName r; | ||
| *r.mutable_namespace_() = kRDK; | ||
| *r.mutable_type() = resource->type().to_string(); | ||
| *r.mutable_name() = resource->name(); | ||
| *r.mutable_subtype() = resource_type; | ||
| *r.mutable_type() = resource_type; | ||
| *r.mutable_subtype() = resource_subtype; |
There was a problem hiding this comment.
@benjirewis - These are the lines I was referring to in my comment.
There was a problem hiding this comment.
Sorry I still must be missing a comment or something; I'm still not sure what your question is.
I realize the code here is a little funky; basically we were creating a resource name with namespace set to "rdk", type set to the ResourceType of the Resource (a field that no longer exists), subtype set to the subtype of the API registration (although we were using a local variable named resource_type), and name set to resource->name(). Now we set namespace to "rdk", type to the type of the API registration, subtype to the subtype of the API registration, and name to resource->name(). I do wonder if I should be setting resource_type to something in the event that (reg->api().to_string() != resource->dynamic_api().to_string()); maybe @stuqdog will know.
There was a problem hiding this comment.
Ah I see. You needed to change it because resource->type() is no longer a thing. At first I didn't see that and I couldn't see what it was that required changes here due to the removal of ResourceType.
I do agree that the code is pretty funky. Hopefully @stuqdog can clarify whether what you have here is OK or needs the handling you suggest, as I'm not certain either.
There was a problem hiding this comment.
Sorry for the slow response. I think what we have here is correct. In the event that (reg->api().to_string() != resource->dynamic_api().to_string()) then the registered model we're looking at is not of the provided resource type and so we continue on line 34. This isn't a case where we're creating a ResourceName that lacks a resource_type but rather a case where we aren't creating a ResourceName at all.
There was a problem hiding this comment.
Ah cool that makes sense! Thanks for context.
It definitely will. I'm just wondering if we know somehow how many modules that might be, and, if so, how many are viam produced (100%?). |
Yeah good idea let me look into that; modules will break even more with #185, so it'll be good to get a sense of which modules we need to fix now. |
|
Gonna wait until we have a list of all C++ modules this change might break before merging. |
cc @acmorrow