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

feat(assert): assertion signatures on more of the assert modules #4140

Closed
wants to merge 10 commits into from

Conversation

syhol
Copy link
Contributor

@syhol syhol commented Jan 10, 2024

Spawned from this conversation #4121 (comment)

Adding more assertion signatures to assert modules.

Background

There are two ways assertion signatures are already implemented in assert/* and thats:

actual unknown - assertInstanceOf & assertIsError & assertStrictEquals:

export function assert???<T>(
  actual: unknown,
  expected: T,
  msg?: string,
): asserts actual is T { /**/ }

This seems to be used for all positive asserts.

and

two type params - assertNotInstanceOf:

export function assert???<A, T extends A>(
  actual: A,
  expected: T,
  msg?: string,
): asserts actual is T { /**/ }

This seems to be uses for all negative/not asserts.

I expect the reason that negative/not asserts use the two type params is because its the only way to do type narrowing by doing asserts actual is Exclude<A, T>.

This PR

In this PR I've implemented assertion signatures on the remaining assert functions that make sense to have assertion signatures, following the pattern thats been previously established:

  • assertEquals using the actual unknown style
  • assertNotEquals and assertNotStrictEquals using the two type params style

@syhol syhol requested a review from kt3k as a code owner January 10, 2024 09:33
@syhol
Copy link
Contributor Author

syhol commented Jan 10, 2024

Addressing this ticket: #4139

@kt3k
Copy link
Member

kt3k commented Jan 11, 2024

Probably this is not a desired change. See the discussion in #2203 for details. It has several benefits to give actual and expected the same generic type T, and this change looses it.

@iuioiua
Copy link
Contributor

iuioiua commented Jan 11, 2024

Yes, I agree with Yoshiya. After seeing this PR, I realised my initial suggestion was wrong. We always want actual and expected to have the same type. You don't need to use either of the assertions if they're not the same type. I'll close this without merging. Either way, thank you, Simon.

@iuioiua iuioiua closed this Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants