RSDK-8557: test module generator output #4
Conversation
| if isinstance(arg.annotation.slice, ast.Subscript): | ||
| if isinstance(arg.annotation.slice.slice, ast.Name) and arg.annotation.slice.slice.id in nodes: | ||
| arg.annotation.slice.slice = return_attribute(resource_name, arg.annotation.slice.slice.id) |
There was a problem hiding this comment.
We have some instances where we have Subscript's in Subscript's (i.e. Dict[str, List[str]]). Since that exists in our Python SDK, I wanted to protect us if it ever happened with a custom class. There is however, the unfortunate truth that if someone decides to put a custom class nested a third time, it will fail. But thankfully this test will fail if that happens.
cli/module_generate.go
Outdated
| IsPublic: false, | ||
| Namespace: "my-org", | ||
| Language: "python", | ||
| Resource: resourceSubtype + resourceType, |
There was a problem hiding this comment.
Not that it matters since it's unused, but doesn't this require a space in between?
| if isinstance(arg.annotation.slice, ast.Subscript): | ||
| if isinstance(arg.annotation.slice.slice, ast.Name) and arg.annotation.slice.slice.id in nodes: | ||
| arg.annotation.slice.slice = return_attribute(resource_name, arg.annotation.slice.slice.id) |
| {{ if eq .ResourceSubtype "mlmodel" }} | ||
| viam-sdk[mlmodel] | ||
| {{ end }} |
There was a problem hiding this comment.
I think this is an if/else: either we do viam-sdk or we do viam-sdk[mlmodel], but i don't think we need both?
| resource = getattr(module, resource_name) | ||
| methods = inspect.getmembers(resource, predicate=inspect.isfunction) | ||
|
|
||
| imports = [] | ||
| abstract_methods = [] | ||
| for name, method in methods: | ||
| for _, method in methods: |
There was a problem hiding this comment.
Do we still need this? Can we get all the imports we need from the AST?
There was a problem hiding this comment.
Yeah, this is something I was wondering and wanted to take out.
AST doesn't seem to show where the types are from. So for example, Vector3 and GeoPoint don't tell us if it's from _pb2 module or not. But I'm not sure how useful this is in the first place because from the limited testing I've done, I saw that these types are usually exported in the __init__.py file so it would be included when we from <component> import *.
There was a problem hiding this comment.
For now I'll take it out and see what happens
cli/module_generate.go
Outdated
| var err error | ||
| resourceSubtype := cCtx.String(moduleFlagResourceSubtype) | ||
| resourceType := cCtx.String(moduleFlagResourceType) | ||
| if resourceSubtype != "" { |
There was a problem hiding this comment.
This should check that both resourceType and resourceSubtype are available
cli/module_generate.go
Outdated
| resourceSubtype := cCtx.String(moduleFlagResourceSubtype) | ||
| resourceType := cCtx.String(moduleFlagResourceType) |
There was a problem hiding this comment.
[nit] extremely minor but could you swap these two lines? i like it so that the broader category (type) comes above the detailed category (subtype)
|
Also, |
Changes:
module generatecommand: resource subtype and resource typeinput,SLAM, andMLModelastinstead ofinspectsensors servicefrom user prompt as it is deprecatedboard componentto user prompt (possible due to theastchange!)mlmodelNOTE:
motionandboardfail in the github workflow. These were tested locally with the changes in this PR, and they work. The tests should pass automatically once the changes are in and the version is updated.