-
Notifications
You must be signed in to change notification settings - Fork 227
chore: add branded model string types #4808
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
Conversation
fc8fcd0
to
93bb11e
Compare
93bb11e
to
79cc657
Compare
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.
Pull Request Overview
Adds branded string types for model identifiers and updates related code to use and enforce these types.
- Introduce new branded types (
ModelName
,ModelEncodedFQN
,ModelPath
,ModelFullPath
, etc.) indomain/models.ts
- Update domain interfaces, worker logic, components, and pages to use branded types and appropriate casts
- Enhance
ModelSQLMeshModel
andLineage
interfaces to reflect the new branded types
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
vscode/react/src/workers/lineage.ts | Cast models arrays and includes checks to ModelEncodedFQN |
vscode/react/src/pages/lineage.tsx | Update ModelSQLMeshModel.update call to use branded model fields |
vscode/react/src/domain/sqlmesh-model.ts | Extend InitialSQLMeshModel and update class properties with branded types |
vscode/react/src/domain/models.ts | Introduce branded string types for model identifiers |
vscode/react/src/domain/lineage.ts | Update Lineage interface to use branded types for models and columns |
vscode/react/src/components/graph/help.ts | Adjust casts for ModelEncodedFQN and ModelName in graph helpers |
vscode/react/src/components/graph/ModelColumns.tsx | Cast lookup keys to ModelName in getColumnFromLineage |
vscode/bus/src/brand.ts | Add Branded type utility |
Comments suppressed due to low confidence (3)
vscode/react/src/domain/lineage.ts:6
- Column keys represent column names, not model names. Consider defining a distinct
ColumnName
type instead of usingModelName
to avoid confusion and improve type clarity.
columns?: Record<ModelName, LineageColumn>
vscode/react/src/domain/sqlmesh-model.ts:28
- The
lineage
record usesModelName
for keys, but top-level lineage entries are keyed by encoded FQNs in other parts of the code. Consider usingModelEncodedFQN
(or a dedicated branded type) to ensure type consistency.
lineage?: Record<ModelName, Lineage>
vscode/react/src/domain/models.ts:16
- [nitpick] The
ModelFQN
,ModelURI
, andModelEncodedURI
types are currently unused. Consider removing them to reduce unnecessary code and maintenance burden.
export type ModelFQN = Branded<string, 'ModelFQN'>
@@ -371,7 +372,7 @@ function mergeLineageWithColumns( | |||
newLineageModelColumnModel, | |||
), | |||
), | |||
).map(encodeURI) | |||
).map((uri: string) => encodeURI(uri)) |
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.
[nitpick] This mapping returns an array of strings but doesn’t cast to ModelEncodedFQN
. For consistency with other code, consider casting each result to ModelEncodedFQN
(e.g., map(uri => encodeURI(uri) as ModelEncodedFQN)
) or using a shared helper function.
).map((uri: string) => encodeURI(uri)) | |
).map((uri: string) => encodeURI(uri) as ModelEncodedFQN) |
Copilot uses AI. Check for mistakes.
@@ -0,0 +1,43 @@ | |||
import type { Branded } from '@bus/brand' |
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.
[nitpick] Consider adding helper functions (e.g., toModelEncodedFQN
) to encapsulate encodeURI
and branded casts. This would reduce repetitive boilerplate and improve readability.
Copilot uses AI. Check for mistakes.
No description provided.