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 request: assertIsNotError #2444

Closed
harrysolovay opened this issue Jul 17, 2022 · 10 comments
Closed

Feat request: assertIsNotError #2444

harrysolovay opened this issue Jul 17, 2022 · 10 comments
Labels
good first issue Good for newcomers suggestion a suggestion yet to be agreed

Comments

@harrysolovay
Copy link
Contributor

Feature request: an assertIsNotError util to compliment assertIsError assertion. I frequently return (not throw) errors.

declare function tryGetThing(): TheThing | TryGetThingError;

To test such function requires us to perform checks to narrow the fn's result union type to exclude its error-based members.

const theThingOrError = tryGetThing();
if (!(theThingOrError instanceof Error)) {
  // `theThingOrError` is not an error within this scope
}

Ideally I could utilize a built-in assertion instead.

const theThingOrError = tryGetThing();
assertIsNotError(theThingOrError);
// `theThingOrError` is no longer an error as of this line
@harrysolovay harrysolovay changed the title Is **not** error assertion Feat request: assertIsNotError Jul 17, 2022
@kt3k kt3k added suggestion a suggestion yet to be agreed good first issue Good for newcomers labels Jul 26, 2022
@cuobiezi
Copy link
Contributor

@harrysolovay There is assertIsError. Is it your want ?

export function assertIsError<E extends Error = Error>(
  error: unknown,
  // deno-lint-ignore no-explicit-any
  ErrorClass?: new (...args: any[]) => E,
  msgIncludes?: string,
  msg?: string,
): asserts error is E {
  if (error instanceof Error === false) {
    throw new AssertionError(`Expected "error" to be an Error object.`);
  }
  if (ErrorClass && !(error instanceof ErrorClass)) {
    msg = `Expected error to be instance of "${ErrorClass.name}", but was "${
      typeof error === "object" ? error?.constructor?.name : "[not an object]"
    }"${msg ? `: ${msg}` : "."}`;
    throw new AssertionError(msg);
  }
  if (
    msgIncludes && (!(error instanceof Error) ||
      !stripColor(error.message).includes(stripColor(msgIncludes)))
  ) {
    msg = `Expected error message to include "${msgIncludes}", but got "${
      error instanceof Error ? error.message : "[not an Error]"
    }"${msg ? `: ${msg}` : "."}`;
    throw new AssertionError(msg);
  }
}

@harrysolovay
Copy link
Contributor Author

As I specified above, this util would be...

[...] to compliment assertIsError assertion.

assertIsError does not provide the desired behavior. I'm looking for the inverse (to assert something is not an error instance).

@cuobiezi
Copy link
Contributor

cuobiezi commented Jul 31, 2022

In my mind about your above case is

const theThingOrError = tryGetThing();
if (!(theThingOrError instanceof Error)) {
  // `theThingOrError` is not an error within this scope
}

// Equal
const theThingOrError = tryGetThing();
if ((theThingOrError instanceof Error)) {
    throw new Error()
}
// `theThingOrError` is not an error within this scope

// Equal
const theThingOrError = tryGetThing();
assertIsError(theThingOrError)
//`theThingOrError` is not an error within this scope

// Or
const theThingOrError = tryGetThing();
if (! assertIsNotError(theThingOrError)) {
  //`theThingOrError` is not an error within this scope
}

Is right ?

@0f-0b
Copy link
Contributor

0f-0b commented Jul 31, 2022

assertNotInstanceOf might be a more general alternative.

export function assertNotInstanceOf<T, U>(
  actual: T,
  Expected: { new (...args: never): unknown; prototype: U },
  message?: string,
): asserts actual is Exclude<T, U> {
  if (actual instanceof Expected) {
    throw new Error(
      message ?? `Expected object to be not an instance of ${Expected.name}`,
    );
  }
}

@harrysolovay
Copy link
Contributor Author

assertNotInstanceOf would be great.

@ghost
Copy link

ghost commented Aug 2, 2022

Is anyone currently working on this?

@iuioiua
Copy link
Collaborator

iuioiua commented Aug 16, 2022

assertNotInstanceOf sounds like a good idea. @kt3k?

@harrysolovay
Copy link
Contributor Author

@iuioiua while I like that you've "deliberately kept simple"... the lack of narrowing is (imo) not ideal. I'd recommend reworking as follows.

- export function assertNotInstanceOf<T extends AnyConstructor>(
+ export function assertNotInstanceOf<A, T>(
- actual: unknown,
+ actual: A,
- unexpectedType: T,
+ unexpectedType: new(...args: any[]) => T,
  msg = `Expected object to not be an instance of "${typeof unexpectedType}"`,
- ) {
+ ): asserts actual is Exclude<A, T> {
  assertFalse(actual instanceof unexpectedType, msg);
}

Without the diff noise:

export function assertNotInstanceOf<A, T>(
  actual: A,
  unexpectedType: new(...args: any[]) => T,
  msg = `Expected object to not be an instance of "${typeof unexpectedType}"`,
): asserts actual is Exclude<A, T> {
  assertFalse(actual instanceof unexpectedType, msg);
}

@iuioiua
Copy link
Collaborator

iuioiua commented Aug 24, 2022

I agree, @harrysolovay. PR submitted.

@kt3k
Copy link
Member

kt3k commented Aug 25, 2022

done in #2558

@kt3k kt3k closed this as completed Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers suggestion a suggestion yet to be agreed
Projects
None yet
Development

No branches or pull requests

5 participants