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(assert): prepare for noUncheckedIndexedAccess #4045

Merged
merged 15 commits into from
Jan 10, 2024

Conversation

syhol
Copy link
Contributor

@syhol syhol commented Dec 31, 2023

Handling all noUncheckedIndexedAccess issues for the assert module, tracked in #4040

@syhol syhol requested a review from kt3k as a code owner December 31, 2023 19:27
assert/_diff.ts Outdated Show resolved Hide resolved
Comment on lines +141 to +151
value: B[b]!,
});
b -= 1;
} else if (type === ADDED) {
result.unshift({
type: swapped ? DiffType.added : DiffType.removed,
value: A[a],
value: A[a]!,
});
a -= 1;
} else {
result.unshift({ type: DiffType.common, value: A[a] });
result.unshift({ type: DiffType.common, value: A[a]! });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel a bit unsure about this one. We can't say for certain if these indices are correct as it's all dependent on if routes was built correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unsure what to do here too. @kt3k, WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

diff is hugely used across Deno ecosystem (via assertEquals). I don't think this includes such basic bugs. I think it's fine to use non null assertion here.

assert/_diff.ts Outdated Show resolved Hide resolved
assert/_diff.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

@syhol syhol left a comment

Choose a reason for hiding this comment

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

Best to review this as 4 separate bits:

  • createCommon: small and simple
  • backTrace: small risk but ultimately ignoring with !
  • createFP / snake / ensureDefined / the bits where currentFP is used: this was a messy situation and the types were lying, now marking FarthestPoint as being possibly undefined in more places, and needing to handle new cases of missing values.
  • diffstr (tokenize / createDetails): a few new consts and type tweaks, nothing crazy

assert/_diff.ts Outdated Show resolved Hide resolved
assert/_diff.ts Show resolved Hide resolved
assert/_diff.ts Show resolved Hide resolved
assert/_diff.ts Outdated Show resolved Hide resolved
assert/_diff.ts Outdated Show resolved Hide resolved
syhol and others added 5 commits January 3, 2024 13:13
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
@syhol
Copy link
Contributor Author

syhol commented Jan 3, 2024

Changing

function (
    slide: FarthestPoint | undefined,
    down: FarthestPoint | undefined,
    foo: any,
) {}

to

function (
    slide?: FarthestPoint,
    down?: FarthestPoint,
    foo: any,
) {}

is causing the error:

A required parameter cannot follow an optional parameter.deno-ts(1016)

Options are:

  1. Revert it back to FarthestPoint | undefined
  2. Move the optional parameters to the end of the parameter list

Personally I prefer option 1 as we never expect to call the function without those parameters. I'll revert it for now but happy to change to option 2 if you wish.

@syhol
Copy link
Contributor Author

syhol commented Jan 3, 2024

Waiting on #4074 to get checks to pass

@iuioiua
Copy link
Collaborator

iuioiua commented Jan 4, 2024

Changing

function (
    slide: FarthestPoint | undefined,
    down: FarthestPoint | undefined,
    foo: any,
) {}

to

function (
    slide?: FarthestPoint,
    down?: FarthestPoint,
    foo: any,
) {}

is causing the error:

A required parameter cannot follow an optional parameter.deno-ts(1016)

Options are:

  1. Revert it back to FarthestPoint | undefined
  2. Move the optional parameters to the end of the parameter list

Personally I prefer option 1 as we never expect to call the function without those parameters. I'll revert it for now but happy to change to option 2 if you wish.

Let's do number 1.

@syhol
Copy link
Contributor Author

syhol commented Jan 4, 2024

Let's do number 1.

Great, thats already applied as of 8aedb7c

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

LGTM too

@iuioiua iuioiua merged commit 7601263 into denoland:main Jan 10, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants