Add typeguard for filtering out undefined #2502
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Kevin-Chant
commented
May 9, 2024
| const sortedFilteredGuardrails = sortAndFilterMetricsByTags( | ||
| guardrailDefs, | ||
| metricFilter | ||
| ); | ||
|
|
||
| const retMetrics = sortedFilteredMetrics | ||
| .map((metricId) => getRow(metricId, false)) | ||
| .filter((row) => row?.metric) as ExperimentTableRow[]; |
There was a problem hiding this comment.
I checked the type definition for what row could be (other than undefined) and it looks like row.metric is always present, so the definition check is just on row itself, hence isDefined should be a drop-in replacement
packages/front-end/components/Settings/Team/SingleRoleSelector.tsx
Outdated
Show resolved
Hide resolved
| @@ -62,8 +62,7 @@ function getExampleAttributes({ | |||
| ? ["foo", "bar"].map((v) => sha256(v, secureAttributeSalt)) | |||
| : ["foo", "bar"]; | |||
| } else if (datatype === "enum") { | |||
| // @ts-expect-error TS(2532) If you come across this, please fix it!: Object is possibly 'undefined'. | |||
| value = enumList.split(",").map((v) => v.trim())[0] ?? null; | |||
| value = enumList?.split(",").map((v) => v.trim())[0] ?? null; | |||
There was a problem hiding this comment.
Tested in node -i that this still behaves appropriately when enumList is either string or undefined:
> "foo"?.split(",").map(v => v.trim())[0] ?? null
'foo'
> undefined?.split(",").map(v => v.trim())[0] ?? null
null
|
Your preview environment pr-2502-bttf has been deployed. Preview environment endpoints are available at: |
jdorn
reviewed
May 10, 2024
jdorn
approved these changes
May 16, 2024
Kevin-Chant
force-pushed
the
kc/add-filter-defined-typeguard
branch
from
15d048a
to
7b35dc9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Features and Changes
Found an easy fix for many instances of TS(2532) across the codebase while working on a separate bug. Added a new shared typeguard
isDefinedwhich will help methods likeArray.prototype.filterto correctly infer the type of the result.Searched the codebase for both instances of 2532 and
filter(Boolean)and updated the usage manually to reduce the likelihood of unintentional bugs. For cases where it wasn't obviously an easy fix, I left the original ts-expect-error and functionality in place.Dependencies
None?
Testing
There's too many files to do manual testing, so instead I'm focusing on thoroughly reading the PR & letting the test suite check for any large-scale issues