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

refactor: consolidate all choose function stuff in util-utils.ts #626

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

rossiam
Copy link
Collaborator

@rossiam rossiam commented Sep 24, 2024

Moved all the supporting types and variables for createChooseFn into the same file as createChooseFn. Not sure why I didn't do this when I created createChooseFn. Also updated affected unit tests.

Copy link

changeset-bot bot commented Sep 24, 2024

⚠️ No Changeset found

Latest commit: a60e2f9

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 move-util-stuff-to-util branch from e8ecd26 to e27f31d Compare September 25, 2024 14:50
@rossiam rossiam changed the title chore: consolidate all choose function stuff in util-utils.ts refactor: consolidate all choose function stuff in util-utils.ts Sep 25, 2024
@rossiam rossiam force-pushed the move-util-stuff-to-util branch from e27f31d to 20ca6d7 Compare September 25, 2024 14:55
@rossiam rossiam force-pushed the move-util-stuff-to-util branch from 20ca6d7 to a60e2f9 Compare September 25, 2024 15:33
import { stringTranslateToId } from '../../../../lib/command/command-util.js'
import {
createChooseFn,
type ChooseFunction,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've recently learned about this type marker (even though it's been around a while!) and have started using it in the PR but I'm realizing now that I've done so rather inconsistently. I plan to do better in the next PR and then open up a discussion with the team in standup to talk about it.

expect(toStringMock).toHaveBeenCalledTimes(1)
expect(toStringMock).toHaveBeenCalledWith()
expect(pushMock).toHaveBeenCalledExactlyOnceWith(['setting', 'setting value'])
expect(toStringMock).toHaveBeenCalledExactlyOnceWith()
Copy link
Contributor

Choose a reason for hiding this comment

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

I checked the Jest documentation but I'm still not quite sure about something. Is this in fact checking that toStringMock was called exactly once with no parameters? Or is it not checking parameters since none were supplied?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A call to a With function that doesn't check the With seems rather silly so checking for no arguments is the only thing that makes sense to me. It's easy enough to test though; I just removed the argument from a call that had one and I get an error. Here's the output when I remove the argument from line 192 above:

 FAIL  src/__tests__/lib/command/util/apps-util.test.ts
  ● buildTableOutput › creates new table with correct options and adds settings

    expect(received).toHaveBeenCalledExactlyOnceWith(expected)

    Expected mock function to have been called exactly once with [], but it was called with ["setting", "setting value"]

      190 | 			expect.objectContaining({ head: ['Key', 'Value'] }),
      191 | 		)
    > 192 | 		expect(pushMock).toHaveBeenCalledExactlyOnceWith()
          | 		                 ^
      193 | 		expect(toStringMock).toHaveBeenCalledExactlyOnceWith()
      194 | 	})
      195 | })

      at Object.<anonymous> (src/__tests__/lib/command/util/apps-util.test.ts:192:20)

Test Suites: 1 failed, 2 passed, 3 total
Tests:       1 failed, 58 passed, 59 total
Snapshots:   0 total
Time:        2.992 s, estimated 6 s
Ran all test suites related to changed files.

@rossiam rossiam merged commit d93bdd5 into SmartThingsCommunity:yargs Sep 27, 2024
4 checks passed
@rossiam rossiam deleted the move-util-stuff-to-util branch September 27, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants