-
Notifications
You must be signed in to change notification settings - Fork 633
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(yaml): make Type
generic
#5394
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5394 +/- ##
=======================================
Coverage 96.35% 96.36%
=======================================
Files 458 458
Lines 37729 37726 -3
Branches 5576 5576
=======================================
Hits 36353 36353
+ Misses 1334 1331 -3
Partials 42 42 ☔ View full report in Codecov by Sentry. |
Type
generic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! Couple of nits.
yaml/_type/set.ts
Outdated
@@ -5,8 +5,7 @@ | |||
|
|||
import type { Type } from "../_type.ts"; | |||
|
|||
// deno-lint-ignore no-explicit-any | |||
function resolveYamlSet(data: any): boolean { | |||
function resolveYamlSet(data: Record<PropertyKey, unknown>): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function resolveYamlSet(data: Record<PropertyKey, unknown>): boolean { | |
function resolveYamlSet(data: ResultType | null): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, that doesn't work, because result type could also be an object or string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should revert this to any as we don't know what types are actually passed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It must be an object or for (const key in data) {...}
would throw no?
Co-authored-by: Yoshiya Hinosawa <[email protected]>
Co-authored-by: Asher Gomez <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Type
any
types