RSDK-2870: use api instead of subtype #824
Conversation
|
Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!
|
stuqdog
left a comment
stuqdog
left a comment
There was a problem hiding this comment.
I definitely think we should revisit whether all these changes need to be breaking and how to avoid breaking when it's easy to do so. Otherwise, this all looks good to me!
src/viam/logging.py
Outdated
| def emit(self, record: logging.LogRecord): | ||
| assert isinstance(record, logging.LogRecord) | ||
| # Fully qualified name of form "{subtype triplet}/{name}", e.g. "rdk:component:arm/myarm" | ||
| # Fully qualified name of form "{API}/{name}", e.g. "rdk:component:arm/myarm" |
There was a problem hiding this comment.
(minor, q) curious why you got rid of "triplet" here? Not a big deal either way, but I think it might be helpful to keep since the way we're using API is a bit of a term of art.
There was a problem hiding this comment.
There was no real reason to get rid of the word "triplet". In other comments, it's referred to simply as API so it was what I did automatically. I can add it back no problem!
src/viam/app/data_client.py
Outdated
| resource_subtype: str | ||
| """ | ||
| deprecated; use `resource_api` | ||
| The resource API. Ex: `rdk:component:sensor` | ||
| """ |
There was a problem hiding this comment.
(minor) I'd love for this to be a property instead, so people can't set it, it's impossible for it to ever drift from the value in resource_api, and so we can emit a deprecation warning on call instead of just including it in comments. Something like
@property
def resource_subtype(self) -> str:
warnings.warn("<some deprecation warning>", DeprecationWarning, stacklevel=2)
return self.resource_api
src/viam/app/data_client.py
Outdated
| DataClient.TabularDataPoint( | ||
| part_id=resp.part_id, | ||
| resource_name=resp.resource_name, | ||
| resource_subtype=resp.resource_subtype, |
There was a problem hiding this comment.
given above comment re: property decorator, I don't think we need to (or should) do this setting here.
stuqdog
left a comment
stuqdog
left a comment
There was a problem hiding this comment.
nice, looks good to me! thanks for all the back and forth on this :)
2 participants
subtypewhen actually referring to the API triplet (i.e.rdk:component:sensor)DataClientfunctions and has a deprecated property inTabularDataPointImportant
BREAKING CHANGES INTRODUCED IN THIS PR
SUBTYPEis nowAPIRegistrymethods have been renamed or contain parameter name changesNote for reviewers: I tried my best to go through all the uses of
subtypethat I could find. Because this is such a loosely used word, it was hard to figure out what was actually asubtypeand what should be anAPIat times, which is the whole point of the change. I wouldn't be surprised if I missed something