feat: add support for virtual devices #318

bflorian · 2022-06-02T13:23:53Z

Added commands for creating virtual devices and generating events on their behalf.

Checklist

I have read the CONTRIBUTING document
Any required documentation has been added
My code follows the code style of this project (npm run lint produces no warnings/errors)
I have added tests to cover my changes

changeset-bot · 2022-06-02T13:23:55Z

🦋 Changeset detected

Latest commit: e8879ab

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@smartthings/cli	Minor
@smartthings/cli-lib	Patch
@smartthings/cli-testlib	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

rossiam · 2022-06-02T16:34:18Z

.changeset/purple-steaks-behave.md

@@ -0,0 +1,7 @@
+---
+"@smartthings/cli": minor


I think we want to stick with patch for all commits so no versions get bumped while we're still considering the CLI beta. @john-u is that correct?

I don't think it will matter while in prerelease, it should not touch the major.minor.patch version number until we promote out of beta.

rossiam · 2022-06-06T15:29:14Z

packages/cli/src/commands/devices/virtual/create.ts

+	static description = 'Creates a virtual device from a device profile ID or definition\n' +
+		'The command can be run interactively, in question & answer mode, with command line parameters,\n' +
+		'or with input from a file or standard in. You can also combine command line input with file input\n' +
+		'so that you can create multiple devices with different names in different locations and rooms using\n' +
+		'the same input file.'


oclif adds two newlines when you include a newline but word wraps on spaces so this looks better if all but the first \n are replaced with spaces.

(Same for other commands that have longer descriptions.)

Also, this text made me think the command would make multiple devices in one go:

You can also combine command line input with file input so that you can create multiple devices with different names in different locations and rooms using the same input file.

I wonder if it wouldn't be better to say something like:

You can also run this command multiple times with the same input file but different command line arguments to create multiple devices with different names in different locations and rooms.

rossiam · 2022-06-06T15:34:58Z

packages/cli/src/commands/devices/virtual/events.ts

+		'$ smartthings devices:virtual:events {id} -i data.yml                                # from a YAML or JSON file',
+		'$ smartthings devices:virtual:events {id} switch:switch on                           # command line input',
+		'$ smartthings devices:virtual:events {id} temperatureMeasurement:temperature 22.5 C  # command line input',


In other commands we've been using angle brackets to represent ids that need to be filled in rather than curly braces so maybe we should stick to that.

rossiam · 2022-06-06T15:53:43Z

packages/cli/src/commands/devices/virtual/events.ts

+		},
+		{
+			name: 'unit',
+			description: 'optional unit of measure',


Since this is optional, it really should be a flag.

Whether or not it is optional depends on the capability and attribute. For example, it is required for the temperatureMeasurement temperature attribute because there is no default temperature unit. It seems strange to make it a flag in that case since the command will fail if it is not provided.

rossiam · 2022-06-06T18:32:46Z

packages/cli/src/commands/devices/virtual/events.ts

+		}
+		await inputAndOutputItem<DeviceEvent[], EventInputOutput>(this, {
+			buildTableOutput: (data: EventInputOutput) => buildTableOutput(this.tableGenerator, data),
+		}, createEvents, inputProcessor(() => true, () => this.getInputFromUser(deviceId)))


getInputFromUser was meant to be the "Q&A" session-type input and getInputFromCommandLine was meant to be for parsing input from the command line. It might be a bigger refactor than you want to do at this point but I think this would be easier to follow and it would be easier to make better error messages if these two were separated, similar to "devices:commands".

rossiam · 2022-06-06T18:41:33Z

packages/cli/src/commands/devices/virtual.ts

+import {allPrototypes, locallyExecutingPrototypes} from './virtual/create-standard'
+
+
+export async function chooseDeviceName(command: APICommand<typeof APICommand.flags>, preselectedName?: string): Promise<string | undefined> {


Recently, we've been putting functions like this in a corresponding <command>-util.ts file, even when they are only used in one place to make writing unit tests easier.

If we think any of the utils here would be helpful in the edge-plugin, we could instead export them from packages/lib/src/device-util.ts

rossiam · 2022-06-06T18:48:30Z

packages/cli/src/commands/devices/virtual.ts

+	return preselectedName
+}
+
+export async function chooseRoom(command: APICommand<typeof APICommand.flags>, locationId: string, preselectedId?: string, autoChoose?: boolean): Promise<string> {


There is already a chooseRoom in rooms-util.ts. It might be worth adding support for autoChoose to that one. It might be worth adding autoChoose to that one and using it.

rossiam · 2022-06-06T19:53:45Z

packages/cli/src/commands/devices/virtual/create-standard.ts

+export const locallyExecutingPrototypes = [
+	{name: 'Switch', id: 'VIRTUAL_SWITCH'},
+	{name: 'Dimmer', id: 'VIRTUAL_DIMMER_SWITCH'},
+	{name: 'Testing devices...', id: 'more'},


This text "Testing devices..." confused me a little when I created a virtual device. Aren't they all testing devices? Would simply "More..." be better?

Well no, virtual switches and dimmers are commonly used in rules for real home applications, whereas the others are only useful for testing, but I agree that "More..." is probably better

john-u · 2022-06-07T14:34:15Z

.changeset/purple-steaks-behave.md

+---
+"@smartthings/cli": minor
+"@smartthings/cli-lib": patch
+"@smartthings/cli-testlib": patch


These changes don't modify testlib, so it doesn't need a release in the changeset.

john-u · 2022-06-07T14:54:01Z

packages/cli/src/commands/devices/virtual.ts

+export interface DeviceProfileDefinition {
+	deviceProfileId?: string
+	deviceProfile?: DeviceProfileCreateRequest
+}


Should this instead be added to the Core SDK?

It seems strange to me to include an interface in the SDK that isn't used by any of the API methods. It's not produced by the swagger code generation, so we'd have to muck around with them to make it something that's used by the methods. It's the design of the CLI command and the way the CLI input flag processing works that makes it necessary.

Yeah I agree. I think it should just be exported from the commands corresponding util module then.

john-u · 2022-06-07T18:50:29Z

.changeset/purple-steaks-behave.md

@@ -0,0 +1,7 @@
+---
+"@smartthings/cli": minor


I don't think it will matter while in prerelease, it should not touch the major.minor.patch version number until we promote out of beta.

john-u · 2022-06-07T18:54:42Z

packages/cli/src/commands/devices/virtual.ts

+import {allPrototypes, locallyExecutingPrototypes} from './virtual/create-standard'
+
+
+export async function chooseDeviceName(command: APICommand<typeof APICommand.flags>, preselectedName?: string): Promise<string | undefined> {


If we think any of the utils here would be helpful in the edge-plugin, we could instead export them from packages/lib/src/device-util.ts

bflorian · 2022-06-08T14:29:15Z

Closing this PR in favor of #327 to preserve the context of these comments since the command name change resulted in so many file names.

bflorian force-pushed the virtual-devices-phase1 branch from eca9d38 to d809e05 Compare June 2, 2022 13:55

bflorian requested review from Sitlintac, john-u and rossiam June 2, 2022 14:17

feat: add support for virtual devices

e8879ab

bflorian force-pushed the virtual-devices-phase1 branch from d809e05 to e8879ab Compare June 7, 2022 11:50

rossiam reviewed Jun 7, 2022

View reviewed changes

john-u reviewed Jun 7, 2022

View reviewed changes

bflorian mentioned this pull request Jun 8, 2022

feat: add support for virtual devices #327

Merged

4 tasks

bflorian closed this Jun 8, 2022

bflorian deleted the virtual-devices-phase1 branch August 3, 2022 21:10

		import {allPrototypes, locallyExecutingPrototypes} from './virtual/create-standard'


		export async function chooseDeviceName(command: APICommand<typeof APICommand.flags>, preselectedName?: string): Promise<string \| undefined> {

feat: add support for virtual devices #318

feat: add support for virtual devices #318

Uh oh!

Conversation

bflorian commented Jun 2, 2022

Checklist

Uh oh!

changeset-bot bot commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bflorian commented Jun 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot bot commented Jun 2, 2022 •

edited

Loading