DOCS-3695, DOCS-3401: Add to dependency, validate, and reconfigure info #4089
Conversation
viambot
added
the
safe to build
✅ Deploy Preview for viam-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| If you do not implement a `Reconfigure` function (see next step), your `Validate` function should also do the following: | ||
|
|
||
| - Get any values from the `config` object that the user has configured. | ||
| - Assign default values as necessary to any optional attributes if the user hasn't configured them.<br><br> |
There was a problem hiding this comment.
I suspect this will work, but personally I think having side effects in Validate() is surprising. I'd put this part in the component's constructor (newMyModularCamera() or whatever it's called). but I don't know what other folks typically do.
There was a problem hiding this comment.
agreed. If reconfigure is unimplemented, I think then the config structure needs to have resource.AlwaysRebuild, and all required attributes should be assigned in the constructor
There was a problem hiding this comment.
I think validate should only be used to confirm that the config is formatted as expected?
There was a problem hiding this comment.
Yeah agreed. I think we should recommend to implement Reconfigure if needing to do these things instead of putting into Validate.
The rest of the changes LGTM! thanks for improving this.
There was a problem hiding this comment.
Michael's not wrong, but I prefer Martha's approach of using AlwaysRebuild as the default, and only writing Reconfigure if you absolutely need it. I kinda want to change the viam module generate template to create an AlwaysRebuild module with a comment saying that if you really need Reconfigure() you can comment out the AlwaysRebuild line. but that's a discussion for a different PR, not this one.
There was a problem hiding this comment.
I guess the first bullet, get any config values, doesn't need to be done in any of these functions since people can just do for example imgFile, err := os.Open(s.cfg.ImagePath) within the actual API function implementation instead of passing around global variables. So that "Get any values..." bullet becomes "If you assigned any config values to global variables, reassign them."?
There was a problem hiding this comment.
Ok, sorry for the more philosophical comments above. I think that they apply to this but the docs are correct as they are in this PR. Just didn't see it earlier. It LGTM!
There was a problem hiding this comment.
Philosophy can be necessary when we are deciding how to guide users! I appreciate the discussion.
| - Assign default values as necessary to any optional attributes if the user hasn't configured them. | ||
| - If your module has dependencies, get the dependencies from the `dependencies` map and cast each resource according to which API it implements, as in [this <file>ackermann.py</file> example](https://github.com/mcvella/viam-ackermann-base/blob/main/src/ackermann.py). | ||
|
|
||
| When the user changes the configuration, the `reconfigure` function is called. |
There was a problem hiding this comment.
[optional nit] maybe make this the first sentence so we start by explaining what the function is doing
| - Assign any default values as necessary to any optional attributes if the user hasn't configured them.<br><br> | ||
| - Get any values from the `config` object that the user has configured. | ||
| - Assign default values as necessary to any optional attributes if the user hasn't configured them. | ||
| - If your module has dependencies, get the dependencies from the `dependencies` map and cast each resource according to which API it implements, as in [this <file>ackermann.py</file> example](https://github.com/mcvella/viam-ackermann-base/blob/main/src/ackermann.py). |
There was a problem hiding this comment.
Maybe show it once instead?
from typing import cast
motors = config.attributes.fields["drive_motors"].list_value
for m in motors:
actual_motor = dependencies[Motor.get_resource_name(m)]
motor = cast(Motor, actual_motor)
There was a problem hiding this comment.
Tried this in staging and I sort of think it takes up too much space and doesn't have enough context around it, esp. since not everyone will have dependencies.
JessamyT
force-pushed
the
3695dependency
branch
from
95b6ea2 to
b742c2b
Compare
|
To get the Go example in the expander to actually include constructor edits, I added an "example variable" |
JessamyT
force-pushed
the
3695dependency
branch
from
9852f6b to
8ead039
Compare
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Viam Docs (https://docs.viam.com)' |
Incremental; still considering a dependencies how-to or similar
@penguinland Mind reviewing the Go part? Edit: adding @martha-johnston per offline discussion
@michaellee1019 for Python part (or any other thoughts)
https://deploy-preview-4089--viam-docs.netlify.app/operate/get-started/other-hardware/#implement-the-component-api