-
Notifications
You must be signed in to change notification settings - Fork 128
feat: add support for virtual devices #327
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
feat: add support for virtual devices #327
Conversation
🦋 Changeset detectedLatest commit: e0d5422 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
6d87aa7 to
9143ca8
Compare
| id: 'switch', | ||
| version: 1, | ||
| status: CustomCapabilityStatus.LIVE, | ||
| name: 'Switch', | ||
| attributes: { | ||
| switch: { | ||
| schema: { | ||
| type: 'object', | ||
| properties: { | ||
| value: { | ||
| title: 'IntegerPercent', | ||
| type: 'string', | ||
| enum: ['on', 'off'], | ||
| }, | ||
| }, | ||
| additionalProperties: false, | ||
| required: [ | ||
| CapabilitySchemaPropertyName.VALUE, | ||
| ], | ||
| }, | ||
| enumCommands: [], | ||
| }, | ||
| }, | ||
| commands: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of including all properties, we have also been using a pattern of stubbing an object with only the properties required by the test case and casting the result to the required type. Here is an example.
It makes the tests less verbose and easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 , though for some reason I had to cast the shortened version to unknown before casting it to Capability. Not sure I understand what that wasn't necessary for the Device example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is expected. TypeScripts assertion rules are conservative and more complex objects will hit this. Documented here.
| export type SelectingConfig<L> = Sorting & Naming & CommonListOutputProducer<L> | ||
|
|
||
| export const indefiniteArticleFor = (name: string): string => name.match(/^[aeiou]/i) ? 'an' : 'a' | ||
| export const indefiniteArticleFor = (name: string): string => name.match(/^[aeio]/i) ? 'an' : 'a' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good catch.
packages/cli/src/__tests__/lib/commands/virtualdevices/virtualdevices-util.test.ts
Outdated
Show resolved
Hide resolved
john-u
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please perform a Format Modified Lines on these changes.
9143ca8 to
4c84d6b
Compare
| }, | ||
| }, | ||
| }, | ||
| } as unknown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make sure to finish the cast in these cases. as unknown as <type>
| }, | ||
| }, | ||
| }, | ||
| } as unknown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as unknown as <type>
| }, | ||
| }, | ||
| }, | ||
| } as unknown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as unknown as <type>
This can be disregarded since we merged #330 |
4c84d6b to
a700daf
Compare
|
I pushed the lint fixes |
| export const locallyExecutingPrototypes = [ | ||
| { name: 'Switch', id: 'VIRTUAL_SWITCH' }, | ||
| { name: 'Dimmer', id: 'VIRTUAL_DIMMER_SWITCH' }, | ||
| { name: 'More...', id: '' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The omission of an id here causes the selection to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang! Good catch. I went back and forth on whether to have a value there and managed to wind up in an inconsistent state. Just pushed the fix
a700daf to
e0d5422
Compare
Added support for creating and generating events for virtual devices.
Note: Because of all the file name changes, this is a new PR replaces #318, which will be closed.
Checklist
npm run lintproduces no warnings/errors)