Skip to content

Conversation

addaleax
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

Looks good!

// support for a newly introduced driver option when it is being added
// to the driver API.
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const __typecheck1: (typeof allTransactionOptions)[number] = '' as Exclude<keyof TransactionOptions, keyof CommandOperationOptions>;
Copy link
Collaborator

@gribnoysup gribnoysup Mar 17, 2022

Choose a reason for hiding this comment

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

Don't have a strong preference really, but it took me awhile to grasp what's exactly is going on there, at the same time I used something like this before (which provides the same result and also doesn't require eslint-disable):

    function assertAllOptionsUsed(options: Exclude<keyof TransactionOptions, keyof CommandOperationOptions>) {
          // These typechecks might look weird, but will tell us if we are missing
          // support for a newly introduced driver option when it is being added
          // to the driver API.
    }
    assertAllOptionsUsed(allTransactionOptions)

EDIT: yikes, I think this particular snippet doesn't work, sorry, but the idea would be to use a noop function to do the assertion instead of using as operator. But now I'm thinking that maybe I'm just misunderstanding how this works and so feel free to ignore this 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, your suggestion is basically to use a function call instead of an assignment, right? That’s easy enough to do, updated the code here :) Still needs eslint comments, though, because the argument is unused.

@addaleax addaleax merged commit 95ad859 into main Mar 17, 2022
@addaleax addaleax deleted the 1151-dev branch March 17, 2022 12:16
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