RSDK-11340: Combine Resource.FromRobot and Resource.FromDependencies #5315
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ies and FromRobot with GetResource
dhritinaidu
requested review from
a team,
njooma and
stuqdog
and removed request for
a team
viambot
added
the
safe to test
|
Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!
|
viambot
added
safe to test
… implemented by al "Providers"
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
njooma
requested changes
Sep 26, 2025
There was a problem hiding this comment.
Nice!!! A few requests:
- You're recreating a lot of functionality in
GetResource, when the implementers already have ways to do it (e.g.Lookup). Just call the existing methods from withinGetResource. - You're marking methods as deprecated (good thing!). You should provide the alternative so that users know what to use instead
resource/resource.go
Outdated
|
|
||
| // GetResource implements Provider for Dependencies by looking up a resource by name. | ||
| func (d Dependencies) GetResource(name Name) (Resource, error) { | ||
| res, err := d.Lookup(name) |
There was a problem hiding this comment.
You can probably just return d.Lookup(name) instead of doing all the error checking
robot/impl/local_robot.go
Outdated
|
|
||
| // GetResource implements resource.Provider for a localRobot by looking up a resource by name. | ||
| func (r *localRobot) GetResource(name resource.Name) (resource.Resource, error) { | ||
| res, err := r.ResourceByName(name) |
There was a problem hiding this comment.
Similarly, just return r.ResourceByName(name) instead of doing all the checks
robot/impl/resource_manager_test.go
Outdated
|
|
||
| // GetResource implements resource.Provider for a dummyRobot by looking up a resource by name. | ||
| func (rr *dummyRobot) GetResource(name resource.Name) (resource.Resource, error) { | ||
| res, err := rr.ResourceByName(name) |
robot/client/client.go
Outdated
|
|
||
| // GetResource implements resource.Provider for a RobotClient by looking up a resource by name. | ||
| func (rc *RobotClient) GetResource(name resource.Name) (resource.Resource, error) { | ||
| res, err := rc.ResourceByName(name) |
testutils/inject/robot.go
Outdated
|
|
||
| // GetResource calls the injected ResourceByName or the real version of GetResource. | ||
| func (r *Robot) GetResource(name resource.Name) (resource.Resource, error) { | ||
| r.Mu.RLock() |
There was a problem hiding this comment.
just call r.ResourceByName(...) here, and that'll take care of checking if there's an override
| type Dependencies map[Name]Resource | ||
|
|
||
| // FromDependencies returns a named component from a collection of dependencies. | ||
| // Deprecated: FromDependencies returns a named component from a collection of dependencies. |
There was a problem hiding this comment.
Provide the new function to use if marking as deprecated
viambot
added
safe to test
viambot
added
safe to test
stuqdog
reviewed
Oct 1, 2025
components/switch/switch.go
Outdated
| } | ||
|
|
||
| // FromDependencies is a helper for getting the named button component from a collection of dependencies. | ||
| // Deprecated: FromDependencies is a helper for getting the named button component from a collection of dependencies. |
There was a problem hiding this comment.
(minor, flyby request) totally unrelated to your PR, but this looks like a bad historical copy/paste. Admittedly this is now deprecated but probably good to still change this from button to switch.
Comment on lines
26
to
29
| // FromRobot is a helper for getting the named Gizmo from the given Robot. | ||
| // Deprecated: FromRobot is a helper for getting the named Gizmo from the given Robot. | ||
| // Use FromProvider instead. | ||
| // | ||
| //nolint:revive // ignore exported comment check |
There was a problem hiding this comment.
(minor) Since this is example code and we aren't really concerned about breaking changes, it's probably better to just remove FromRobot entirely and make sure the example shows current best practices.
Same for the Summation api.
testutils/inject/robot.go
Outdated
|
|
||
| // GetResource calls the injected ResourceByName or the real version of GetResource. | ||
| func (r *Robot) GetResource(name resource.Name) (resource.Resource, error) { | ||
| return r.ResourceByNameFunc(name) |
There was a problem hiding this comment.
(minor) probably better to return r.ResourceByName(name), which checks if ResourceByNameFunc is nil and acts accordingly.
viambot
added
safe to test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New Interface called Provider with a GetResource method.
GetResource is implemented in all the Resources respectively to either fetch the named resource from a collection of dependencies or from the given robot based on what was passed in.
All instances of Resource.FromRobot and Resource.FromDependencies were replaced with Resource.GetResource.