refactor(encoding): prepare for noUncheckedIndexedAccess
#4275
Conversation
eryue0220
changed the title
noUncheckedIndexedAccessnoUncheckedIndexedAccess
There was a problem hiding this comment.
The aim of these PRs should be to fix any possible runtime bugs due to undeclared keys on objects. So, the changes should be justified based on runtime behaviour rather than just applying type castings to make the TypeScript checker happy.
For example, there should be a justification behind each time a non-null assertion is used. Comments on these changes would also help us (the maintainers) understand the reasons behind them. This mostly applies to non-trivial changes. Trivial changes don't require comments.
Can you review this and related PRs and let us know once they're ready? If you'd like a guide on how these PRs should be approached, PTAL at previously merged ones by @syhol, who provides justifications behind changes: https://github.com/denoland/deno_std/pulls?q=is%3Apr+author%3Asyhol+is%3Aclosed+noUncheckedIndexedAccess
encoding/ascii85.ts
Outdated
| @@ -114,10 +114,10 @@ export function encodeAscii85( | |||
| } | |||
| break; | |||
| case "RFC 1924": | |||
| output = output.map((val) => rfc1924[val.charCodeAt(0) - 33]); | |||
| output = output.map((val) => rfc1924[val.charCodeAt(0) - 33]) as string[]; | |||
There was a problem hiding this comment.
| output = output.map((val) => rfc1924[val.charCodeAt(0) - 33]) as string[]; | |
| output = output.map((val) => rfc1924[val.charCodeAt(0) - 33]!); |
Ditto where appropriate. Non-null assertions are a little cleaner than type castings.
There was a problem hiding this comment.
Yeah, but I think it's better to express what type it's, if it just only one type, except undefined. And that's why I used as more than !.
There was a problem hiding this comment.
Type-castings make more sense if the variable has more than one non-null type. E.g. string | number | undefined.
encoding/ascii85_test.ts
Outdated
| @@ -204,7 +206,7 @@ Deno.test({ | |||
|
|
|||
| for (const [input, expect] of tests) { | |||
| assertEquals( | |||
| decodeAscii85(input), | |||
| decodeAscii85(input as string), | |||
There was a problem hiding this comment.
This change is unnecessary if the tests variable is declared using as const (const assertion). This fact should be checked whenever a non-changing variable is used.
There was a problem hiding this comment.
@eryue0220, I've gone ahead and applied the necessary tweaks. Now, this PR LGTM. @kt3k, PTAL.
|
Thank you very much, I just finished my lunar new year vocation. |
…0/deno_std into fix/expect-custom-equality-case * 'fix/expect-custom-equality-case' of github.com:eryue0220/deno_std: (63 commits) docs: link to `assertThrows()` and `assertRejects()` (denoland#4395) chore(log): sync `level` and `levelName` in BaseHandler (denoland#4393) docs: ignore bad snippet examples (denoland#4388) chore(media_types): format test names (denoland#4380) docs: clarify underscore guidance in README (denoland#4385) feat(collections): add `pick` and `omit` (denoland#4218) chore(msgpack): format test names (denoland#4381) refactor(encoding): prepare for `noUncheckedIndexedAccess` (denoland#4275) refactor(streams): prepare for `noUncheckedIndexedAccess` (denoland#4377) chore: fix .editorconfig syntax (denoland#4376) chore(semver): remove legacy `Range.ranges` object definition (denoland#4374) chore(semver): move breaking versions (denoland#4372) refactor(semver): rename `comparatorFormat()` to `formatComparator()` (denoland#4373) test(semver): add test for parse_range (denoland#4345) chore: use 'release' event for triggering jsr publish (denoland#4370) chore(http): fix spawned tests after migration script (denoland#4368) chore(crypto): move test scripts to own files (denoland#4367) 0.217.0 (denoland#4369) build: update _ to - in workspace converter script (denoland#4357) chore(media_types): move `extensions` utility (denoland#4358) ...
Ref: #4040