Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RSDK-9726 - improve modulegen help text and deprecate resource-type flag #4711

Merged
merged 7 commits into from
Jan 16, 2025

Conversation

stuqdog
Copy link
Member

@stuqdog stuqdog commented Jan 14, 2025

Provides improved help text in terms of the usage invocation, as well as clarifying the role/usage of the language, model-name, name, public-namespace, and resource-subtype flags.

Additionally, removes resource-type as a flag we pay attention to/care about. While still supported for backwards compatibility, it is invisible and ignored. This flag is a gotcha since each subtype only has one valid type (except generic, but we can carve out a specific case for that), and a user shouldn't reasonably be expected to understand the component/service distinction that is primarily internal.

Tested by creating modules using both old and new code and diffing them to ensure no changes in behavior.

Old help text:

NAME:
   viam module generate - generate a new modular resource via prompts

USAGE:
   viam module generate [command options] [arguments...]

OPTIONS:
   --enable-cloud            generate Github workflows to build module (default: false)
   --language value          language to use for module
   --model-name value        resource model name to use in module
   --name value              name to use for module
   --public                  set module to public (default: false)
   --public-namespace value  namespace or organization ID of module
   --register                register module with Viam to associate with your organization (default: false)
   --resource-subtype value  resource subtype to use in module
   --resource-type value     resource type to use

New help text:

NAME:
   viam module generate - generate a new modular resource via prompts

USAGE:
   viam module generate --name=<name> --language=<language> --resource-subtype=<resource-subtype> --model-name=<model-name> --public-namespace=<public-namespace> [other options]

OPTIONS:
   --enable-cloud            generate Github workflows to build module (default: false)
   --language value          language to use for module, can be one of [python, go]
   --model-name value        name for the particular resource subtype implementation. for example, a sensor model that detects moisture might be named 'moisture'
   --name value              name to use for module. for example, a module that contains sensor implementations might be named 'sensors'
   --public                  set module to public (default: false)
   --public-namespace value  namespace or organization ID of module. must be either a valid organization ID, or a namespace that exists within a user organization
   --register                register module with Viam to associate with your organization (default: false)
   --resource-subtype value  resource subtype to use in module, for example arm, camera, or motion. see https://docs.viam.com/dev/reference/glossary/#term-subtype for more details

@stuqdog stuqdog requested a review from a team as a code owner January 14, 2025 17:35
@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jan 14, 2025
Comment on lines +108 to +109
// make sure we support subtypes that are passed with different spacers (e.g.,
// "power sensor", "power-sensor", "power_sensor", "powerSensor")
Copy link
Member Author

@stuqdog stuqdog Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit of flyby logic here. previously, the CLI would complain if we passed power-sensor but not power_sensor. Changes here enable us to be more case-agnostic for multi-word resource subtypes.

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 14, 2025
@stuqdog
Copy link
Member Author

stuqdog commented Jan 14, 2025

cc @ale7714

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 14, 2025
@stuqdog
Copy link
Member Author

stuqdog commented Jan 14, 2025

Probably gonna hold off on merging this until #4692 is in, as that feature changes some formatting stuff and I want to ensure consistency. cc @purplenicole730

Copy link
Member

@ale7714 ale7714 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great! and thank you for thinking about resource type/subtype a little bit more in depth, it's good to be aware of this

Copy link
Member

@purplenicole730 purplenicole730 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also update the module generation workflow?

cli/module_generate/modulegen/inputs.go Show resolved Hide resolved
@stuqdog
Copy link
Member Author

stuqdog commented Jan 15, 2025

Could you also update the module generation workflow?

Sure! Maybe dumb question, but what do you want to see updated? it seems to me that we want to keep the workflow basically the same except stop asking for unnecessary information, which I think this PR achieves?

@purplenicole730
Copy link
Member

purplenicole730 commented Jan 16, 2025

Could you also update the module generation workflow?

Sure! Maybe dumb question, but what do you want to see updated? it seems to me that we want to keep the workflow basically the same except stop asking for unnecessary information, which I think this PR achieves?

I'm pretty sure the workflow still uses the flag you're trying to stop using, so I think it'd be good to also follow that there!

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 16, 2025
@stuqdog
Copy link
Member Author

stuqdog commented Jan 16, 2025

I'm pretty sure the workflow still uses the flag you're trying to stop using, so I think it'd be good to also follow that there!

@purplenicole730 I think we should be good on that! It isn't visible as a flag (though it's still supported of course for backwards compatibility), and the promptUser version already had subtype and type combined into a single field.

Screenshots:
Screenshot 2025-01-16 at 14 22 42
Screenshot 2025-01-16 at 14 36 57

@stuqdog
Copy link
Member Author

stuqdog commented Jan 16, 2025

Oh though actually there was an issue where we were listing Generic Component Component and Generic Service Service instead of just Generic {type} in the user prompt, which is now fixed :)

@stuqdog
Copy link
Member Author

stuqdog commented Jan 16, 2025

Possibly I'm misunderstanding but just to clarify: at no point are we using the flag at this point. You can confirm this by the removal of ResourceType as field of the moduleGenArgs type. We do still care about resource type of course and it is still a field on ModuleInputs, but (like the Resource field of ModuleInputs), it is inferred based on what was actually asked for rather than being directly asked for (since while we care about what it is, we don't care about a user knowing what it is).

@stuqdog
Copy link
Member Author

stuqdog commented Jan 16, 2025

Just spoke offline, @purplenicole730 clarified that it was the CI workflow that needed updating :) which is now done!

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 16, 2025
Copy link
Member

@purplenicole730 purplenicole730 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woo!

@stuqdog stuqdog force-pushed the improve-module-generate-help branch from 3bb7874 to 5257803 Compare January 16, 2025 20:55
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 16, 2025
@stuqdog stuqdog merged commit a57278c into viamrobotics:main Jan 16, 2025
16 checks passed
@stuqdog stuqdog deleted the improve-module-generate-help branch January 16, 2025 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants