Skip to content

miscellaneous enhancements to createChooseFn #724

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Apr 22, 2025

Conversation

rossiam
Copy link
Collaborator

@rossiam rossiam commented Apr 17, 2025

miscellanous createChooseFn cleanup

  • pass through an instance of APICommand instead of SmartThingsClient which allows functions access to things in APICommand (and SmartThingsCommand) like the logger
  • Added type to imports here and there that I noticed were missing it
  • fixed a bug where the function created by createChooseFn was not using the passed-in listItems function
  • added support for config defaults to createChooseFn
  • updated relevant unit tests

@rossiam rossiam requested a review from a team April 17, 2025 19:09
Copy link

changeset-bot bot commented Apr 17, 2025

⚠️ No Changeset found

Latest commit: 7c0b992

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@rossiam rossiam force-pushed the createChooseFn-cleanup branch from a021d5c to 8c1b0e1 Compare April 17, 2025 19:11
expect.objectContaining({ listItems: expect.any(Function) }),
)

// expect listItemsMock to get called when listItems passed to third argument of selectFromListMock is called
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this TODO?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, yeah, I've added that test now.


if (opts.useConfigDefault) {
if (!createOptions?.defaultValue) {
throw Error('invalid state, choose function called with "useConfigDefault" but no default configured')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this change would be appropriate for an error message, but it might be easier to parse if it said something like "choose function was called" because it's a little ambiguous whether "choose" is a verb or an adjective here. (I.e., that "choose function" is a type of function, not an instruction)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a developer-targeted error message, so anything that makes more sense to us is good. I've changed it to:

invalid state, the choose<Thing> function was called with "useConfigDefault" but no default configured

@rossiam rossiam force-pushed the createChooseFn-cleanup branch from 8c1b0e1 to 7c0b992 Compare April 22, 2025 18:25
@rossiam rossiam merged commit f71a33c into SmartThingsCommunity:yargs Apr 22, 2025
4 checks passed
@rossiam rossiam deleted the createChooseFn-cleanup branch April 22, 2025 18:29
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants