Support model versions #1436
Support model versions #1436
Conversation
WalkthroughThe changes in this pull request involve modifications across multiple files to standardize the methods used for retrieving nodes from the Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
src/manifest/dbtProject.ts
Outdated
| if (nodeMetaMap.get(splits[2])?.config.materialized === "ephemeral") { | ||
| // this will ignore version | ||
| if ( | ||
| nodeMetaMap.lookupByBaseName(splits[2])?.config.materialized === |
There was a problem hiding this comment.
nodeMetaMap. lookupByUniqueId(n.key)
src/manifest/dbtProject.ts
Outdated
| return this.dbtProjectIntegration.getBulkCompiledSQL( | ||
| models | ||
| .map((m) => nodeMetaMap.get(m.split(".")[2])) | ||
| .map((m) => nodeMetaMap.lookupByBaseName(m.split(".")[2])) |
There was a problem hiding this comment.
nodeMetaMap.lookupByUniqueId(m)
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 95ed52a in 1 minute and 9 seconds
More details
- Looked at
653lines of code in24files - Skipped
0files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. src/commands/index.ts:721
- Draft comment:
Ensure thatlookupByBaseNamecorrectly handles model versions. This change replaces direct map access with a method call that considers model versions, which is crucial for supporting versioned models. - Reason this comment was not posted:
Confidence changes required:20%
The PR aims to support model versions by changing the way models are accessed in the code. The changes involve replacing direct map access with method calls that handle model versions. This is done by introducing a new interfaceNodeMetaMapwith methods likelookupByBaseName,lookupByUniqueId, andnodes. The implementation of this interface,NodeMetaMapImpl, manages the mapping of model names and unique IDs to their metadata, considering versions. The changes are consistent across the codebase, ensuring that model versions are supported wherever models are accessed. The PR seems to address the issue of supporting model versions effectively.
2. src/commands/validateSql.ts:109
- Draft comment:
Ensure thatlookupByBaseNamecorrectly handles model versions. This change replaces direct map access with a method call that considers model versions, which is crucial for supporting versioned models. - Reason this comment was not posted:
Confidence changes required:20%
The PR introduces a new methodlookupByBaseNameto handle model versions. This method is used in various places to replace direct map access, ensuring that the correct version of a model is retrieved. This change is crucial for supporting versioned models, as it abstracts the logic of handling versions away from the rest of the codebase.
3. src/definition_provider/modelDefinitionProvider.ts:128
- Draft comment:
Ensure thatlookupByBaseNamecorrectly handles model versions. This change replaces direct map access with a method call that considers model versions, which is crucial for supporting versioned models. - Reason this comment was not posted:
Confidence changes required:20%
The PR introduces a new methodlookupByBaseNameto handle model versions. This method is used in various places to replace direct map access, ensuring that the correct version of a model is retrieved. This change is crucial for supporting versioned models, as it abstracts the logic of handling versions away from the rest of the codebase.
4. src/hover_provider/modelHoverProvider.ts:150
- Draft comment:
Ensure thatlookupByBaseNamecorrectly handles model versions. This change replaces direct map access with a method call that considers model versions, which is crucial for supporting versioned models. - Reason this comment was not posted:
Confidence changes required:20%
The PR introduces a new methodlookupByBaseNameto handle model versions. This method is used in various places to replace direct map access, ensuring that the correct version of a model is retrieved. This change is crucial for supporting versioned models, as it abstracts the logic of handling versions away from the rest of the codebase.
5. src/services/dbtTestService.ts:101
- Draft comment:
Ensure thatlookupByBaseNamecorrectly handles model versions. This change replaces direct map access with a method call that considers model versions, which is crucial for supporting versioned models. - Reason this comment was not posted:
Confidence changes required:20%
The PR introduces a new methodlookupByBaseNameto handle model versions. This method is used in various places to replace direct map access, ensuring that the correct version of a model is retrieved. This change is crucial for supporting versioned models, as it abstracts the logic of handling versions away from the rest of the codebase.
6. src/services/docGenService.ts:223
- Draft comment:
Ensure thatlookupByBaseNamecorrectly handles model versions. This change replaces direct map access with a method call that considers model versions, which is crucial for supporting versioned models. - Reason this comment was not posted:
Confidence changes required:20%
The PR introduces a new methodlookupByBaseNameto handle model versions. This method is used in various places to replace direct map access, ensuring that the correct version of a model is retrieved. This change is crucial for supporting versioned models, as it abstracts the logic of handling versions away from the rest of the codebase.
7. src/treeview_provider/modelTreeviewProvider.ts:111
- Draft comment:
Ensure thatlookupByBaseNamecorrectly handles model versions. This change replaces direct map access with a method call that considers model versions, which is crucial for supporting versioned models. - Reason this comment was not posted:
Confidence changes required:20%
The PR introduces a new methodlookupByBaseNameto handle model versions. This method is used in various places to replace direct map access, ensuring that the correct version of a model is retrieved. This change is crucial for supporting versioned models, as it abstracts the logic of handling versions away from the rest of the codebase.
8. src/manifest/dbtProject.ts:1168
- Draft comment:
Use 'environment variables' instead of 'env vars' for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment does not appear to be related to any changes in the diff. It seems to be a generic suggestion that does not apply to the current context. Therefore, it should be removed.
I might be missing some context where 'env vars' is used, but based on the provided content, it doesn't seem relevant.
The absence of 'env vars' in the diff and the file content supports the decision to remove the comment.
The comment should be deleted as it is not related to any changes made in the diff.
9. src/manifest/dbtProject.ts:1237
- Draft comment:
Use semantic versioning (semver) for version comparisons instead of direct array index comparisons. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment seems to be misplaced as it does not directly relate to the changes made in the diff. The change is about using a different method to access node metadata, not about version comparison. The comment should be removed as it does not address a change made in this diff.
I might be missing some context about how the version is being handled elsewhere in the code, but based on the diff, the comment does not seem relevant.
The focus should be on the changes made in the diff. Since the comment does not relate to these changes, it should be removed.
Remove the comment as it does not pertain to the changes made in the diff.
10. src/manifest/dbtProject.ts:1293
- Draft comment:
Use semantic versioning (semver) for version comparisons instead of direct array index comparisons. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_9H0OvkXefw3pln9o
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (24)
src/commands/tests/missingSchemaTest.ts (1)
Line range hint
23-51: Enhance model identification and error reporting for versioned modelsTo fully support versioned models as per the PR objectives, consider the following improvements:
Update the model identification logic to account for versioned models. The current check
value.uniqueId.startsWith(DBTProject.RESOURCE_TYPE_MODEL)might need adjustment.Modify the error message to include version information if present. For example:
const modelName = value.name; const versionInfo = modelName.match(/_v(\d+)$/); const errMessage = versionInfo ? `Documentation missing for model: ${modelName} (version ${versionInfo[1]})` : `Documentation missing for model: ${modelName}`;
- Consider adding version information to the
missingDocsDictset to differentiate between versions of the same model.These changes would make the function more aware of versioned models, improving error reporting and potentially helping with future features related to model versioning.
src/commands/tests/unmaterializedModelTest.ts (1)
Line range hint
29-35: Consider updating modelKey construction for versioned modelsTo support versioned dbt models (e.g.,
<model_file_name>_v2.sql), consider updating themodelKeyconstruction. Currently, it doesn't account for version information, which might lead to issues when dealing with multiple versions of the same model.Consider modifying the
modelKeyconstruction to include version information:const modelKey = JSON.stringify({ projectroot: projectRootUri.fsPath, project: projectName, database: value.database.toLowerCase(), schema: value.schema.toLowerCase(), name: value.alias.toLowerCase(), + version: extractVersionFromName(value.name), // New function to extract version });You'll need to implement the
extractVersionFromNamefunction to parse the version from the model name. This change would allow the system to distinguish between different versions of the same model.Additionally, consider updating the error message to include version information:
-const errMessage = `Model ${value.name} does not exist in the database`; +const errMessage = `Model ${value.name} (version ${extractVersionFromName(value.name) || 'N/A'}) does not exist in the database`;These changes would make the
UnmaterializedModelTestmore aware of versioned models, aligning with the PR's objective.src/commands/tests/undocumentedModelColumnTest.ts (2)
Line range hint
31-43: Suggestion: Enhance versioning support in modelKey constructionTo fully support versioned models as mentioned in the PR objectives, consider modifying the
modelKeyconstruction to include version information. This change would help distinguish between different versions of the same model.Here's a suggested modification:
const modelKey = JSON.stringify({ projectroot: projectRootUri.fsPath, project: projectName, database: value.database.toLowerCase(), schema: value.schema.toLowerCase(), name: value.alias.toLowerCase(), version: extractVersionFromName(value.name) // New field }); // Helper function to extract version (implement this) function extractVersionFromName(name: string): string | null { // Example implementation: const match = name.match(/_v(\d+)$/); return match ? match[1] : null; }Also, consider adding error handling for cases where the structure of
valuemight be different for versioned models:try { // Existing code for handling model and columns } catch (error) { console.error(`Error processing model ${value.name}: ${error.message}`); // Consider adding this error to diagnostics }These changes will make the function more robust in handling versioned models and provide better error reporting if issues arise.
🧰 Tools
🪛 Biome
[error] 24-25: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
1-85: Overall suggestions for improvementWhile the changes made are a step in the right direction for supporting versioned models, there are several areas where the code could be further improved:
Method Refactoring: The
runmethod is quite long. Consider breaking it down into smaller, more focused methods for better readability and maintainability.Error Handling: Implement more robust error handling throughout the method, especially when dealing with potentially undefined or null values.
Performance Optimization: Look into optimizing the multiple iterations and object key checks. Consider using more efficient data structures or memoization techniques where appropriate.
Versioning Support: Fully implement versioning support by modifying how model keys are constructed and how column checks are performed for versioned models.
Type Safety: Consider using more TypeScript features to enhance type safety, especially when dealing with the
valueobject and its properties.Documentation: Add JSDoc comments to the class and method to explain their purpose and expected inputs/outputs.
Here's a high-level suggestion for refactoring:
@provideSingleton(UndocumentedModelColumnTest) export class UndocumentedModelColumnTest implements AltimateScanStep { public async run(scanContext: ScanContext) { const { project, catalog, eventMap, diagnostics, scanResults } = this.validateScanContext(scanContext); const nodes = this.getValidNodes(eventMap, scanResults); for (const node of nodes) { const modelKey = this.constructModelKey(project, node); if (this.isModelInCatalog(catalog, modelKey)) { this.checkUndocumentedColumns(node, catalog[modelKey], project, diagnostics); } } } private validateScanContext(scanContext: ScanContext): ValidatedScanContext { /* ... */ } private getValidNodes(eventMap: EventMap, scanResults: ScanResults): Node[] { /* ... */ } private constructModelKey(project: Project, node: Node): string { /* ... */ } private isModelInCatalog(catalog: Catalog, modelKey: string): boolean { /* ... */ } private checkUndocumentedColumns(node: Node, modelDict: ModelDict, project: Project, diagnostics: Diagnostics): void { /* ... */ } }This structure would make the code more modular, easier to test, and more maintainable. It would also provide clear points for implementing versioning support and error handling.
🧰 Tools
🪛 Biome
[error] 24-25: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/hover_provider/modelHoverProvider.ts (1)
Line range hint
1-156: Summary and Next StepsThe change to use
lookupByBaseNameinstead ofgetin theModelHoverProviderclass is a good step towards supporting versioned dbt models. However, to ensure full compatibility and maintain code quality, consider the following next steps:
- Review other parts of the codebase that interact with model names to ensure they also support versioned models.
- Update the
README.mdas mentioned in the PR checklist to document this new feature.- Add or update tests to cover scenarios with both versioned and non-versioned model names.
- Consider adding logging or telemetry to track usage of versioned models, which could help in future development decisions.
To further improve the architecture:
- Consider creating a utility function or class for handling model name versioning, which could be reused across the codebase.
- Evaluate if the
NodeMetaMapinterface needs to be updated to explicitly support versioned models.These steps will help ensure that the support for versioned models is comprehensive and well-integrated into the existing codebase.
src/commands/tests/staleModelColumnTest.ts (1)
Line range hint
1-138: Consider versioned dbt models in the implementationWhile the changes made are valid, they don't directly address the PR objective of supporting versioned dbt models. To fully support this feature, consider the following suggestions:
- In the
modelKeycreation (around line 76), add logic to handle version suffixes. For example:const modelName = value.alias.toLowerCase(); const baseModelName = modelName.replace(/_v\d+$/, ''); // Remove version suffix if present const modelKey = JSON.stringify({ // ... other properties ... name: baseModelName, version: modelName !== baseModelName ? modelName.split('_').pop() : undefined, });
Update the column comparison logic to account for potential differences between versions. You might need to fetch and compare columns from all versions of a model.
Modify the error message for missing columns (around line 101) to consider version mismatches:
const errMessage = `Column ${existingCol} listed in model ${value.name} is not found in the database. It may be outdated, misspelled, or belong to a different version of the model.`;
- Consider adding a method to detect and handle version conflicts between different versions of the same base model.
These changes would help align the implementation with the PR objective of supporting versioned dbt models.
src/commands/sqlToModel.ts (1)
Line range hint
33-165: Consider refactoringgetModelFromSqlfor improved readabilityWhile not directly related to the current change, the
getModelFromSqlmethod is quite long and complex. Consider refactoring it into smaller, more focused methods to improve readability and maintainability. This could also make it easier to add specific handling for versioned models in the future.Here's a suggested outline for refactoring:
- Extract the project and event validation into a separate method.
- Move the SQL compilation logic into its own method.
- Separate the model conversion logic from the UI update logic.
Example refactoring (partial):
private validateProjectAndEvent(currentFilePath: vscode.Uri): { project: DBTProject, event: ManifestCacheProjectAddedEvent } { const project = this.dbtProjectContainer.findDBTProject(currentFilePath); if (!project) { throw new Error("Could not find a dbt project. Please put the new model in a dbt project before converting to a model."); } const event = this.eventMap.get(project.projectRoot.fsPath); if (!event) { project.throwDiagnosticsErrorIfAvailable(); throw new Error("The extension is still initializing, please retry again."); } return { project, event }; } private async compileSql(project: DBTProject, fileText: string): Promise<string> { try { return await project.unsafeCompileQuery(fileText); } catch (error) { throw new Error("Could not compile the SQL: " + (error as Error).message); } } async getModelFromSql() { if (!this.altimate.handlePreviewFeatures()) { return; } this.telemetry.sendTelemetryEvent("sqlToModel"); const activeEditor = window.activeTextEditor; if (!activeEditor) { return; } try { const { project, event } = this.validateProjectAndEvent(activeEditor.document.uri); const fileText = activeEditor.document.getText(); const compiledSql = await this.compileSql(project, fileText); // Continue with the rest of the logic... } catch (error) { window.showErrorMessage(extendErrorWithSupportLinks((error as Error).message)); this.telemetry.sendTelemetryError("sqlToModelError"); } }This refactoring will make the code more modular and easier to maintain, especially when adding support for versioned models.
src/domain.ts (1)
Line range hint
1-11: Consider similar interface changes for other Map-based types and update documentation.While the new
NodeMetaMapinterface is a good improvement, I noticed that other similar Map-based types (e.g.,MacroMetaMap,MetricMetaMap) weren't modified. Consider the following suggestions:
- Evaluate if similar interface changes would be beneficial for other Map-based types, especially if they might need versioning support in the future.
- Update the project documentation to reflect the new
NodeMetaMapinterface and its usage.- If you decide to keep other types as-is, document the rationale for the different treatments to maintain clarity for future developers.
Also applies to: 17-279
src/manifest/parsers/graphParser.ts (2)
161-167: Improved model lookup and null safetyThe changes in the "model" case are well-implemented:
- Using
nodeMetaMap.lookupByUniqueId(parentNodeName)instead ofnodeMetaMap.get(nodeName)is a good approach for supporting versioned models, as it allows looking up models by their unique ID.- The null check before accessing the model's path adds robustness to the code, preventing potential runtime errors.
These modifications align well with the PR objectives of supporting versioned dbt models.
Consider using optional chaining to make the code more concise:
const model = nodeMetaMap.lookupByUniqueId(parentNodeName); const url = model?.path; return model ? new Model(model.uniqueId, parentNodeName, url ?? "") : undefined;This approach maintains null safety while reducing the number of lines and improving readability.
170-176: Consistent implementation for seed modelsThe changes in the "seed" case mirror those in the "model" case, which is excellent for maintaining code consistency. The null check adds robustness and addresses the concern raised in the previous review.
For consistency with the suggestion for the "model" case, consider using optional chaining here as well:
const model = nodeMetaMap.lookupByUniqueId(parentNodeName); const url = model?.path; return model ? new Seed(model.uniqueId, parentNodeName, url ?? "") : undefined;This approach maintains null safety while reducing the number of lines and improving readability.
src/services/queryManifestService.ts (1)
153-153: Add a comment explaining the change in behavior.To improve code maintainability and make the change in behavior explicit, consider adding a comment explaining why all nodes are now included. This will help future developers understand the rationale behind this implementation.
Consider adding a comment like this:
// Include all nodes to support versioned models, including analysis models return Array.from(event.nodeMetaMap.nodes()).map((node) => node.uniqueId);src/autocompletion_provider/modelAutocompletionProvider.ts (1)
Line range hint
1-180: Overall assessment: Changes improve code structure but don't address versioned models support.The modifications to the
onManifestCacheChangedmethod simplify the code and improve readability. However, the main objective of supporting versioned dbt models (as mentioned in the PR description and linked issue #709) has not been addressed in these changes.To fully implement support for versioned models:
- Update the
modelAutocompleteMapto include version information.- Modify the
provideCompletionItemsmethod to display and handle versioned models in autocompletion.- Add unit tests to ensure proper handling of both regular and versioned models.
These additional changes will be crucial for resolving the issue and meeting the PR objectives.
Consider creating a separate utility function for parsing model names and extracting version information. This would improve modularity and make it easier to update the version parsing logic in the future if needed.
src/services/dbtTestService.ts (2)
276-276: LGTM! Consider adding versioning-specific debug logging.The change from
gettolookupByBaseNameis consistent with the previous modification and supports the PR objective of handling versioned dbt models.To aid in troubleshooting and verify the correct handling of versioned models, consider adding a debug log statement specifically for versioned models. For example:
const _node = nodeMetaMap.lookupByBaseName(modelName); if (!_node) { this.dbtTerminal.debug("no node for tableName:", modelName); return; } +this.dbtTerminal.debug("dbtTests", "node found for model:", _node.name, "version:", _node.version); const key = _node.uniqueId;This additional logging will help verify that versioned models are being correctly identified and processed.
Line range hint
1-324: Overall implementation looks good. Consider additional areas for versioning support.The changes made to support versioned dbt models are consistent and focused. However, to ensure comprehensive support for versioned models throughout the
DbtTestService, consider the following:
- Review other methods in this file that interact with model names to see if they also need to use
lookupByBaseName.- Update any relevant unit tests to cover scenarios with versioned model names.
- If there's any documentation for this service, ensure it reflects the new support for versioned models.
To fully support model versioning across the extension, you may need to make similar changes in other services or components that interact with model names. Consider creating a utility function or central place to handle model name lookups, which could simplify future maintenance and ensure consistent behavior across the codebase.
src/commands/validateSql.ts (2)
109-109: Consider adding a comment to explainlookupByBaseName.To improve code readability and maintainability, consider adding a brief comment explaining the purpose of the
lookupByBaseNamemethod. This will help future developers understand why this method is used instead of the standardgetmethod, especially in the context of supporting versioned models.Example comment:
// Use lookupByBaseName to support both versioned and non-versioned model names const node = nodeMetaMap.lookupByBaseName(modelName);
Line range hint
37-286: Consider refactoring thevalidateSqlmethod in the future.The
validateSqlmethod is quite long and complex, which may make it difficult to maintain and understand. While not directly related to the current change, it might be beneficial to consider refactoring this method in the future. This could involve breaking it down into smaller, more focused methods to improve readability and maintainability.Some potential refactoring ideas:
- Extract the progress window logic into a separate method.
- Create a dedicated method for compiling the query and fetching parent models.
- Separate the SQL validation logic from the UI update logic.
src/treeview_provider/modelTreeviewProvider.ts (1)
229-229: Consistent model lookup in DocumentationTreeviewProviderThe change to use
lookupByBaseNamein theDocumentationTreeviewProviderclass is consistent with the modification inModelTreeviewProvider. This ensures that versioned dbt models are properly handled in the documentation tree view as well.For consistency, consider updating the variable name from
modelNametofileNameon line 226, similar to how it's named in theModelTreeviewProviderclass. This would make the code more uniform across both providers:-const modelName = path.basename( +const fileName = path.basename( window.activeTextEditor!.document.fileName, ".sql", );src/webview_provider/sqlLineagePanel.ts (4)
145-148: LGTM! Consider adding a comment for clarity.The change from
nodeMetaMap.get(modelName)tonodeMetaMap.lookupByBaseName(modelName)aligns well with the PR objective of supporting model versions. This new method likely provides a more flexible lookup that can handle versioned model names.Consider adding a brief comment explaining the purpose of using
lookupByBaseNameinstead ofget, especially in relation to model versioning support. This would improve code readability and maintainability.
196-198: Excellent comments! Consider refactoring for clarity.The added comments provide valuable insight into the considerations for model versioning, particularly highlighting the potential issues with splitting the
uniqueId. This aligns well with the PR objective and helps future maintainers understand the reasoning behind the code.Consider refactoring this section to make it more robust against future changes:
// Model versions may affect the structure of uniqueId // Avoid manipulating uniqueId to prevent compatibility issues const baseName = splits[splits.length - 1]; const _node = nodeMetaMap.lookupByBaseName(baseName);This refactoring separates the concern of extracting the base name from the lookup operation, making the code more flexible if the
uniqueIdstructure changes in the future.
411-411: LGTM! Consider consistent variable naming.The change from
nodeMetaMap.get(tableName)tonodeMetaMap.lookupByBaseName(tableName)is consistent with the previous modifications and supports the PR objective of handling versioned model names.For consistency with the earlier parts of the code, consider renaming the
tableNamevariable tobaseName:const baseName = splits[splits.length - 1]; const node = nodeMetaMap.lookupByBaseName(baseName);This change would make the code more consistent throughout the file and clearer in its intent.
Line range hint
1-563: Overall, excellent implementation of model version support!The changes in this file consistently replace
nodeMetaMap.get()withnodeMetaMap.lookupByBaseName(), which aligns well with the PR objective of supporting model versions. The added comments provide valuable context for future maintainers.A few minor suggestions have been made to improve clarity and consistency, but overall, the implementation looks solid. These changes should effectively address the issue of handling versioned dbt models within the extension.
To further improve the robustness of this implementation:
- Consider adding unit tests specifically for the
lookupByBaseNamefunctionality to ensure it correctly handles various model naming scenarios.- Update the documentation (README.md or relevant sections) to explain how the extension now supports versioned models.
- If not already done, implement proper error handling for cases where
lookupByBaseNamemight not find a match, to gracefully handle potential edge cases.Great work on implementing this feature!
src/commands/index.ts (1)
Line range hint
1-824: Consider reviewing model name handling throughout the codebaseWhile the change to use
lookupByBaseNameis appropriate for theshowDocumentationcommand, it might be beneficial to review other areas of the codebase where model names are handled. This could ensure consistent support for versioned models across all functionalities.Consider the following steps:
- Review all uses of
nodeMetaMapto ensure they support versioned models.- Update the documentation to reflect the new support for versioned models.
- Add tests to verify that versioned models are correctly handled in various scenarios.
src/manifest/parsers/nodeParser.ts (2)
140-142: Include all lookup maps in debug log for completenessCurrently, the debug log outputs
modelNameLookupMapandmodelMetadataLookupMapbut omitslatestVersionLookupMap. Including all maps can aid in debugging and provide a complete picture of the internal state.Apply this diff to include
latestVersionLookupMap:this.terminal.debug( "NodeParser", `Returning nodes for "${project.getProjectName()}" at ${project.projectRoot}`, modelNameLookupMap, modelMetadataLookupMap, + latestVersionLookupMap, );
83-83: Avoid potential variable shadowing withversionThe variable
versionis extracted fromnodesMap. Ensure that it doesn't unintentionally shadow any variables from an outer scope, which could lead to unexpected behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (24)
- src/autocompletion_provider/modelAutocompletionProvider.ts (1 hunks)
- src/commands/index.ts (1 hunks)
- src/commands/sqlToModel.ts (1 hunks)
- src/commands/tests/missingSchemaTest.ts (1 hunks)
- src/commands/tests/staleModelColumnTest.ts (1 hunks)
- src/commands/tests/undocumentedModelColumnTest.ts (1 hunks)
- src/commands/tests/unmaterializedModelTest.ts (1 hunks)
- src/commands/validateSql.ts (1 hunks)
- src/comment_provider/conversationProvider.ts (1 hunks)
- src/definition_provider/modelDefinitionProvider.ts (1 hunks)
- src/domain.ts (1 hunks)
- src/hover_provider/macroHoverProvider.ts (1 hunks)
- src/hover_provider/modelHoverProvider.ts (1 hunks)
- src/hover_provider/utils.ts (1 hunks)
- src/manifest/dbtProject.ts (3 hunks)
- src/manifest/parsers/graphParser.ts (6 hunks)
- src/manifest/parsers/index.ts (2 hunks)
- src/manifest/parsers/nodeParser.ts (5 hunks)
- src/services/dbtTestService.ts (2 hunks)
- src/services/docGenService.ts (1 hunks)
- src/services/queryManifestService.ts (2 hunks)
- src/treeview_provider/modelTreeviewProvider.ts (2 hunks)
- src/webview_provider/newLineagePanel.ts (3 hunks)
- src/webview_provider/sqlLineagePanel.ts (3 hunks)
🔇 Additional comments (33)
src/commands/tests/missingSchemaTest.ts (1)
20-20: Approve simplification, but clarification needed on versioning supportThe change to iterate directly over
nodeMetaMap.nodes()is a good simplification, as the previous key was unused. However, it's unclear how this change relates to supporting model versions as mentioned in the PR objectives.Could you clarify how this change contributes to supporting versioned dbt models? This would help in understanding the full context of the modification.
Additionally, there's a TODO comment about filtering out models that might be relevant to the versioning issue. Consider addressing this TODO in the context of model versioning. For example, you might need to handle both versioned and non-versioned model names when filtering.
To verify the impact of this change, let's check for any other occurrences of
nodeMetaMapusage:This will help ensure that the change is consistent with other parts of the codebase that might interact with
nodeMetaMap.✅ Verification successful
Change Verified: Iteration Simplification is Consistent
The modification to iterate directly over
nodeMetaMap.nodes()aligns with existing usages across the codebase. No issues were found related to supporting versioned dbt models or other functionalities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other usages of nodeMetaMap to ensure consistency rg --type typescript 'nodeMetaMap' srcLength of output: 73
Script:
#!/bin/bash # Description: Search for other usages of nodeMetaMap to ensure consistency rg -g "*.ts" 'nodeMetaMap' srcLength of output: 5991
src/commands/tests/unmaterializedModelTest.ts (1)
21-21: Clarify the impact of changing the iteration methodThe iteration method has been changed from (presumably)
for (const [key, value] of nodeMetaMap)tofor (const value of nodeMetaMap.nodes()). While this simplifies the loop if thekeywasn't needed, it raises a few questions:
- Was the
keyused anywhere in the original implementation? If so, how is that functionality being maintained?- Is
nodeMetaMap.nodes()a custom method? How does its return value differ from the original Map entries?- How does this change relate to supporting versioned dbt models, which is the main objective of this PR?
To ensure no functionality is lost, let's check if the
keywas used elsewhere in this file:Additionally, to understand the
nodeMetaMapstructure:Please provide more context on how this change supports the PR's objective of handling versioned dbt models.
✅ Verification successful
Iteration Method Change Verified
The modification from
for (const [key, value] of nodeMetaMap)tofor (const value of nodeMetaMap.nodes())is safe. Thekeyvariable was not used in the original loop, so its removal does not affect functionality. Additionally,nodeMetaMapis properly defined and utilized consistently across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of 'key' variable in the file rg '\bkey\b' src/commands/tests/unmaterializedModelTest.tsLength of output: 60
Script:
#!/bin/bash # Search for the definition or type of nodeMetaMap rg 'nodeMetaMap' src/commands/tests/Length of output: 762
src/hover_provider/macroHoverProvider.ts (1)
84-84: Approve change with verification requestsThis change from
nodeMetaMap.values()tonodeMetaMap.nodes()appears to support the PR objective of handling versioned dbt models. It's a good step towards addressing the issue.However, to ensure correctness and maintainability:
- Please verify that the
nodes()method is properly implemented on theNodeMetaMaptype and returns the expected data structure, including versioned models.- Consider adding a comment explaining why this change was made and how it supports versioned models.
- Update any relevant documentation to reflect this change in node retrieval.
- Ensure that tests cover both versioned and non-versioned model scenarios to validate the change.
To verify the implementation and usage of the
nodes()method:src/commands/tests/undocumentedModelColumnTest.ts (1)
22-22: Approved: Iteration change looks good, but needs verificationThe change from iterating over
nodeMetaMapentries to usingnodeMetaMap.nodes()seems to be a step in the right direction for supporting versioned models. It simplifies the code and potentially makes it more flexible for different node structures.However, to ensure this change fully addresses the versioning support:
- Could you confirm that
nodeMetaMap.nodes()correctly returns all nodes, including those for versioned models?- Are there any cases where we might need information from the keys that were previously available in the
for...ofloop?To verify the impact of this change, please run the following script:
This will help us understand the broader context of
nodeMetaMapusage and ensure the change is consistent with its implementation across the codebase.✅ Verification successful
Verified: Iteration Change Confirmed
The modification from iterating over
nodeMetaMapentries to usingnodeMetaMap.nodes()has been thoroughly reviewed. All existing usages ofnodeMetaMap.nodes()across the codebase indicate that relying solely on node values does not omit any necessary information previously accessed via keys. This change enhances code readability and aligns with the objective of supporting versioned models without introducing functional discrepancies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the existence and usage of nodeMetaMap.nodes() method # Test 1: Check if nodeMetaMap.nodes() is defined elsewhere in the codebase echo "Checking for nodeMetaMap.nodes() definition:" rg --type typescript "nodeMetaMap\s*=.*" -A 10 # Test 2: Check for other usages of nodeMetaMap.nodes() in the codebase echo "Checking for other usages of nodeMetaMap.nodes():" rg --type typescript "nodeMetaMap\.nodes\(\)" # Test 3: Look for any comments or documentation about nodeMetaMap structure echo "Checking for nodeMetaMap documentation:" rg --type typescript "nodeMetaMap" -C 5Length of output: 538
Script:
#!/bin/bash # Description: Check for the existence and usage of nodeMetaMap.nodes() method # Test 1: Check if nodeMetaMap.nodes() is defined elsewhere in the codebase echo "Checking for nodeMetaMap.nodes() definition:" rg "nodeMetaMap\s*=.*" -A 10 --glob "*.ts" # Test 2: Check for other usages of nodeMetaMap.nodes() in the codebase echo "Checking for other usages of nodeMetaMap.nodes():" rg "nodeMetaMap\.nodes\(\)" --glob "*.ts" # Test 3: Look for any comments or documentation about nodeMetaMap structure echo "Checking for nodeMetaMap documentation:" rg "nodeMetaMap" -C 5 --glob "*.ts"Length of output: 41765
src/hover_provider/utils.ts (1)
92-92: Conditional approval: Clarify the purpose of usingnodes()instead ofvalues()The change from
[...event.nodeMetaMap.values()]to[...event.nodeMetaMap.nodes()]appears to be a modification in how node data is accessed. While this change might be valid, it raises a few points that need clarification:
- Could you explain the rationale behind this change? It doesn't seem directly related to the PR's objective of supporting model versions.
- Has the
nodes()method been implemented and tested on thenodeMetaMapobject? If not, this could lead to runtime errors.- Are there any performance implications of using
nodes()instead ofvalues()?To ensure this change doesn't introduce any issues, please:
- Verify that the
nodes()method is correctly implemented on thenodeMetaMapobject.- Update any relevant unit tests to cover this change in data access.
- Consider adding a comment explaining why
nodes()is preferred overvalues()in this context.This script will help verify the implementation and usage of the
nodes()method across the codebase.src/definition_provider/modelDefinitionProvider.ts (1)
128-128: Approve change with verification requestThe change from
nodeMap.get(modelName)tonodeMap.lookupByBaseName(modelName)appears to address the issue of supporting versioned dbt models (e.g.,<model_file_name>_v2.sql) as described in the PR objectives. This modification should allow for more flexible model name lookups.To ensure the robustness of this change, please verify the following:
- Confirm that the
lookupByBaseNamemethod is correctly implemented in theNodeMetaMapclass.- Ensure that
lookupByBaseNamehandles both versioned and non-versioned model names appropriately.- Add or update tests to cover this new functionality, including edge cases for versioned and non-versioned models.
To assist with verification, you can run the following script to locate the
NodeMetaMapclass definition and itslookupByBaseNamemethod:This will help ensure that the change is implemented correctly and maintains compatibility with existing functionality.
✅ Verification successful
To resolve the issue with the previous shell script, please run the following updated script to locate the
NodeMetaMapclass and thelookupByBaseNamemethod:
Verification Successful
The
lookupByBaseNamemethod is correctly implemented in theNodeMetaMapImplclass and is utilized across multiple parts of the codebase, aligning with the PR objective to support versioned dbt models.To ensure full robustness:
- Confirm Test Coverage: Verify that there are tests covering the
lookupByBaseNamemethod, including scenarios for both versioned and non-versioned model names.- Review Implementation Details: Ensure that
lookupByBaseNamehandles edge cases and maintains existing functionality for non-versioned models.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Locate NodeMetaMap class and lookupByBaseName method # Find NodeMetaMap class definition echo "NodeMetaMap class definition:" rg --type typescript "class NodeMetaMap" -A 5 # Find lookupByBaseName method echo "\nlookupByBaseName method:" rg --type typescript "lookupByBaseName.*\(" -A 5Length of output: 300
Script:
#!/bin/bash # Description: Locate NodeMetaMap class and lookupByBaseName method without relying on --type # Find NodeMetaMap class definition in .ts and .tsx files echo "NodeMetaMap class definition:" rg "class NodeMetaMap" --glob "*.ts" --glob "*.tsx" -A 5 # Find lookupByBaseName method in .ts and .tsx files echo "\nlookupByBaseName method:" rg "lookupByBaseName\s*\(" --glob "*.ts" --glob "*.tsx" -A 5Length of output: 10518
src/hover_provider/modelHoverProvider.ts (1)
150-150: Approve change with suggestions for further improvementsThe modification from
nodeMap.get(modelName)tonodeMap.lookupByBaseName(modelName)appears to address the issue of supporting versioned dbt models. This change allows the hover provider to find the correct node even when the model name includes a version suffix.While this change is a step in the right direction, consider the following suggestions to ensure robustness:
Add a comment explaining the purpose of using
lookupByBaseName, e.g.:// Use lookupByBaseName to support versioned model names (e.g., model_v2.sql) const node = nodeMap.lookupByBaseName(modelName);Consider adding error handling or logging for cases where
lookupByBaseNamemight return undefined for valid model names that aren't versioned.Update unit tests (if they exist) to cover both versioned and non-versioned model names.
To ensure this change doesn't introduce any regressions, please run the following verification:
This will help ensure that the
lookupByBaseNamemethod is implemented correctly and that there are no remaining instances ofnodeMap.get()that should be updated. It will also help locate any relevant test files that may need updating.src/commands/tests/staleModelColumnTest.ts (1)
Line range hint
1-138: Summary and Request for ClarificationThe changes made to the
StaleModelColumnTestclass, particularly in therunmethod, appear to be a performance optimization. However, it's not clear how these changes directly support the PR objective of handling versioned dbt models.To fully address the PR objectives:
- Consider implementing the suggestions provided earlier to handle versioned model names and potential version conflicts.
- Provide more context on how the current changes contribute to supporting versioned dbt models.
- If there are other files changed in this PR that more directly address the versioning support, please highlight their relationship to this file.
To help understand the full scope of changes related to model versioning, please run the following script:
#!/bin/bash # Description: Find all files changed in this PR that mention versioning or contain version-related logic # Expected result: A list of files that are relevant to the versioning feature git diff --name-only main | xargs rg -l 'version|_v\d+'This will help identify other relevant files that might need review in the context of supporting versioned dbt models.
src/commands/sqlToModel.ts (1)
74-74: Approve change, but verify impact on versioned modelsThe change from
nodeMetaMap.values()tonodeMetaMap.nodes()appears to be intentional and potentially related to supporting versioned dbt models. However, it's important to ensure this change fully addresses the issue.To verify the impact of this change:
- Please confirm that
nodeMetaMap.nodes()includes versioned models (e.g.,model_v2.sql).- Ensure that this change, combined with any other modifications not visible in this file, fully implements support for versioned models as described in issue Support for Model Versioning #709.
Additionally, consider adding a comment explaining the reason for this change to improve code maintainability.
src/domain.ts (1)
12-16: Approve the new NodeMetaMap interface with suggestions for verification.The new
NodeMetaMapinterface is a good improvement over the previous Map-based type alias. It provides more specific methods that align well with the PR objective of supporting versioned models. ThelookupByBaseNamemethod, in particular, seems tailored for this purpose.Some points to consider:
- The interface allows for more flexible and potentially more efficient implementations.
- It provides clear methods for common operations, improving code readability and maintainability.
To ensure this change is properly integrated, please run the following verification script:
This script will help identify any areas in the codebase that might need updates due to this interface change.
src/manifest/parsers/graphParser.ts (2)
Line range hint
48-52: Improved efficiency in graph parsingThe removal of the
uniquefunction and direct use of thenodesarray in theparents,children,tests, andmetricsmappings is a good optimization. This change simplifies the code and potentially improves performance by avoiding unnecessary array filtering.This modification aligns well with the PR objectives, as it could enhance the handling of versioned models by ensuring all nodes are processed without unintended deduplication.
Also applies to: 66-70, 85-89, 104-108
Line range hint
1-207: Summary of changes in GraphParserThe changes made to the
GraphParserclass generally improve support for versioned dbt models, which aligns well with the PR objectives. Key improvements include:
- Optimized graph parsing by removing unnecessary array filtering.
- Updated model lookup methods to support versioned models.
- Added null checks in some cases to prevent runtime errors.
However, there are some inconsistencies in null checking across different node types. Addressing these inconsistencies will further improve the robustness of the code.
Overall, the changes are a step in the right direction for supporting model versioning. Once the suggested improvements are implemented, particularly the consistent null checking across all node types, the code will be more reliable and maintainable.
src/services/queryManifestService.ts (2)
135-137: Improved method signature with explicit return type.The updated method signature with an explicit return type
Iterable<string> | undefinedenhances type safety and code readability. This change aligns well with TypeScript best practices.
153-153: Clarify the removal of RESOURCE_TYPE_ANALYSIS filter.The previous implementation filtered out models of type
DBTProject.RESOURCE_TYPE_ANALYSIS. Can you explain the rationale behind removing this filter? While this change might support versioned models as per the PR objectives, it could potentially impact other parts of the system that expect analysis models to be excluded.To assess the impact of this change, please run the following script:
This script will help us understand if there are other parts of the codebase that might be affected by this change.
src/autocompletion_provider/modelAutocompletionProvider.ts (1)
142-155:⚠️ Potential issueSimplified code structure looks good, but versioned models support is missing.
The changes to use
nodes()instead ofentries()and the simplified filtering and mapping logic improve code readability. However, the current implementation doesn't address the main objective of supporting versioned dbt models.To support versioned models, consider the following improvements:
- Modify the filtering logic to identify versioned models (e.g., those ending with
_v2,_v3, etc.).- Update the
modelNamein the mapping to include version information.- Adjust the
CompletionItemgeneration inprovideCompletionItemsmethod to display version information.Here's a suggested implementation:
const versionRegex = /_v\d+$/; const models = added.nodeMetaMap.nodes(); this.modelAutocompleteMap.set( added.project.projectRoot.fsPath, Array.from(models) .filter( (model) => model.resource_type !== DBTProject.RESOURCE_TYPE_ANALYSIS ) .map((model) => { const versionMatch = model.name.match(versionRegex); const baseModelName = versionMatch ? model.name.replace(versionRegex, '') : model.name; const version = versionMatch ? versionMatch[0] : ''; return { projectName, packageName: model.package_name, modelName: baseModelName, version: version, }; }), );To ensure these changes work as expected, please add unit tests that cover:
- Regular (non-versioned) models
- Versioned models (e.g.,
model_v2,model_v3)- Edge cases (e.g., models with
_vin the name but not at the end)src/manifest/parsers/index.ts (3)
Line range hint
1-258: Implementation details for versioned model supportThe changes to import and use
NodeMetaMapImplare a step in the right direction for supporting versioned dbt models. However, the PR description lacks details about how this implementation actually handles versioned models.Could you please provide more information on:
- How
NodeMetaMapImpldifferentiates between versioned and non-versioned models?- Are there any changes in how model names are parsed or stored to account for version suffixes (e.g.,
_v2.sql)?- Are there any other files changed as part of this implementation that we should review?
To help verify the implementation, please run the following script:
#!/bin/bash # Description: Search for version-related logic in the codebase # Test: Look for version-related logic in NodeMetaMapImpl rg --type typescript -A 5 'class NodeMetaMapImpl' # Test: Search for handling of versioned model names rg --type typescript 'model.*version|version.*model' # Test: Check for any other files that might be part of this implementation fd -e ts -e js | xargs rg 'NodeMetaMapImpl|versioned.*model|model.*version'
11-11: LGTM! Verify NodeMetaMapImpl implementation.The addition of
NodeMetaMapImplto the import statement is a good step towards supporting versioned dbt models. This new implementation likely introduces functionality for handling different versions of nodes.To ensure the
NodeMetaMapImplis correctly implemented, please run the following script:
58-58: LGTM! Verify impact on ManifestParser.The change from
new Map()tonew NodeMetaMapImpl()fornodeMetaMapinitialization is consistent with the import changes and aligns with the goal of supporting versioned dbt models.To ensure this change doesn't negatively impact the rest of the
ManifestParserclass, please run the following script:✅ Verification successful
To gather more information about the usage of nodeMetaMap and NodeMetaMapImpl in the codebase, I'll generate some shell scripts to search for relevant code patterns.
To gather the remaining information needed for our verification, I'll generate some additional shell scripts to search for relevant code patterns.
LGTM. Change is correct and compatible.
The change from
new Map()tonew NodeMetaMapImpl()fornodeMetaMapinitialization in the ManifestParser class is correct and consistent with the import statement we reviewed earlier. The NodeMetaMapImpl provides a more specialized interface for managing node metadata, including version tracking and lookups by both base name and unique ID.While the usage of nodeMetaMap is widespread across the codebase, the interface provided by NodeMetaMapImpl should maintain compatibility with the previous Map implementation. The NodeMetaMapImpl appears to offer all the necessary functionality of the simple Map, with additional features for version management.
This change should not negatively impact the rest of the ManifestParser class or the codebase. The specialized implementation may even provide benefits in terms of version management and more efficient lookups.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of nodeMetaMap in ManifestParser # Test: Check if nodeMetaMap is used consistently throughout the ManifestParser class ast-grep --lang typescript --pattern 'class ManifestParser { $$$ nodeMetaMap $$$ }' # Test: Verify if there are any remaining references to the old Map() initialization rg --type typescript 'nodeMetaMap.*new Map\(\)'Length of output: 168226
Script:
#!/bin/bash # Search for nodeMetaMap usage in ManifestParser class echo "nodeMetaMap usage in ManifestParser:" rg "class ManifestParser" -A 20 | rg "nodeMetaMap" # Check for any remaining Map() initializations echo "\nRemaining Map() initializations:" rg "new Map\(\)" --type ts # Examine NodeMetaMapImpl implementation echo "\nNodeMetaMapImpl implementation:" rg "class NodeMetaMapImpl" -A 20 # Check for other usages of NodeMetaMapImpl echo "\nOther usages of NodeMetaMapImpl:" rg "NodeMetaMapImpl" --type tsLength of output: 7551
Script:
#!/bin/bash # Search for ManifestParser class definition and its contents echo "ManifestParser class definition and contents:" rg "class ManifestParser" -A 50 # Search for any usage of nodeMetaMap in the entire codebase echo "\nUsage of nodeMetaMap in the codebase:" rg "nodeMetaMap" --type ts # Search for methods in NodeMetaMapImpl echo "\nMethods in NodeMetaMapImpl:" rg "class NodeMetaMapImpl" -A 50Length of output: 13012
src/services/dbtTestService.ts (1)
101-101: LGTM! Verify error handling for versioned models.The change from
gettolookupByBaseNamealigns with the PR objective of supporting versioned dbt models. This new method should correctly handle model names with version suffixes (e.g.,model_name_v2.sql).To ensure robust error handling, please verify that
lookupByBaseNamebehaves correctly for both versioned and non-versioned models. Consider running the following script to check its implementation:src/commands/validateSql.ts (1)
109-109: LGTM! This change supports versioned dbt models.The modification from
nodeMetaMap.get(modelName)tonodeMetaMap.lookupByBaseName(modelName)is a good solution for supporting versioned dbt models. This change allows the extension to correctly handle both versioned (e.g.,model_name_v2.sql) and non-versioned model names, addressing the issue described in #709.src/treeview_provider/modelTreeviewProvider.ts (2)
111-115: Improved model lookup for versioned dbt modelsThe changes in the
getChildrenmethod ofModelTreeviewProviderclass effectively address the issue of supporting versioned dbt models. By usinglookupByBaseNameinstead of constructing a unique identifier, the code now handles various naming conventions, including versioned models like<model_file_name>_v2.sql.This approach is more flexible and aligns with the PR objective of supporting model versions. It also gracefully handles cases where no model is found by returning an empty array.
Line range hint
1-487: Summary: Effective implementation of versioned dbt model supportThe changes in this file successfully address the issue of supporting versioned dbt models (issue #709). The modifications in both
ModelTreeviewProviderandDocumentationTreeviewProviderclasses use a more flexible approach to model lookup, allowing the extension to work correctly with versioned models like<model_file_name>_v2.sql.Key improvements:
- Use of
lookupByBaseNamefor model identification.- Consistent implementation across different tree view providers.
- Minimal and targeted changes, reducing the risk of introducing new issues.
These changes align well with the PR objectives and should resolve the stated issue. The implementation is clean and maintains the existing structure of the code.
src/services/docGenService.ts (1)
223-223: LGTM! VerifylookupByBaseNameimplementation.The change from
gettolookupByBaseNameappears to address the issue of supporting versioned dbt models. This modification allows the extension to handle model names with version suffixes (e.g.,<model_file_name>_v2.sql).To ensure the
lookupByBaseNamemethod is correctly implemented, please run the following verification script:This script will help confirm that the
lookupByBaseNamemethod is properly defined and used consistently across the codebase.✅ Verification successful
Verified:
lookupByBaseNameis properly implemented and utilized across the codebase.The
lookupByBaseNamemethod is correctly defined and used insrc/services/docGenService.tsand other relevant TypeScript files, ensuring support for versioned dbt models as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of lookupByBaseName method # Test: Search for the lookupByBaseName method definition rg --type typescript -A 10 'lookupByBaseName\s*\(' # Test: Check for any usage of lookupByBaseName method rg --type typescript 'lookupByBaseName\s*\('Length of output: 169
Script:
#!/bin/bash # Description: Verify the implementation of lookupByBaseName method without using --type # Search for the lookupByBaseName method definition in .ts and .tsx files rg 'lookupByBaseName\s*\(' 'src/**/*.ts' 'src/**/*.tsx' # Check for any usage of lookupByBaseName method in .ts and .tsx files rg 'lookupByBaseName\s*\(' 'src/**/*.ts' 'src/**/*.tsx'Length of output: 325
Script:
#!/bin/bash # Description: Broadly search for the lookupByBaseName method implementation across all TypeScript and JavaScript files # Find all .ts and .tsx files in the repository fd --type f --extension ts --extension tsx > ts_files.txt # Search for the lookupByBaseName method definition in the found TypeScript files rg 'lookupByBaseName\s*\(' $(<ts_files.txt) # Additionally, search in .js and .jsx files in case the project uses JavaScript fd --type f --extension js --extension jsx > js_files.txt rg 'lookupByBaseName\s*\(' $(<js_files.txt)Length of output: 21970
src/comment_provider/conversationProvider.ts (1)
437-437: Verify the impact of usinglookupByBaseNamefor node metadata retrievalThe change from
get(resourceName)tolookupByBaseName(resourceName)appears to address the PR objective of supporting versioned dbt models. This modification likely allows the retrieval of node metadata for models with versioned names (e.g.,model_name_v2.sql).However, please consider the following points:
- Ensure that this change doesn't break functionality for non-versioned models.
- Verify that
lookupByBaseNamehandles cases where the base name might not be unique (e.g.,model_v1.sqlandmodel_v2.sqlwould have the same base name).- Consider adding error handling for cases where
lookupByBaseNamemight return undefined.- If possible, add unit tests to cover both versioned and non-versioned model scenarios.
To ensure this change works as expected, please run the following verification script:
This script will help verify the implementation and usage of
lookupByBaseName, as well as check for related tests and documentation.src/webview_provider/newLineagePanel.ts (4)
468-468: LGTM: ReplacedgetwithlookupByBaseNameThe change from
nodeMetaMap.get(tableName)tonodeMetaMap.lookupByBaseName(tableName)appears to be an improvement in the node lookup mechanism. This change likely addresses the issue of supporting versioned dbt models, as mentioned in the PR objectives.
843-843: LGTM: Consistent use oflookupByBaseNameThe change from
nodeMetaMap.get(table)tonodeMetaMap.lookupByBaseName(table)is consistent with the previous change. This modification ensures that the new lookup method is used throughout the class, maintaining consistency and supporting versioned models.
937-937: LGTM: AppliedlookupByBaseNameto starting node retrievalThe change from
nodeMetaMap.get(tableName)tonodeMetaMap.lookupByBaseName(tableName)in thegetStartingNodemethod ensures that the new lookup mechanism is also used when retrieving the initial node for the lineage panel. This change is consistent with the previous modifications and supports the goal of handling versioned models.
Line range hint
468-937: Summary of changes and their implicationsThe changes made in this file consistently replace the
getmethod withlookupByBaseNamewhen retrieving nodes from thenodeMetaMap. This modification appears to address the issue of supporting versioned dbt models, as mentioned in the PR objectives.Implications of these changes:
- Improved support for versioned models: The
lookupByBaseNamemethod likely handles cases where model names include version suffixes (e.g.,<model_file_name>_v2.sql).- Consistent behavior: The change is applied uniformly across different parts of the
NewLineagePanelclass, ensuring consistent handling of model lookups.- Backwards compatibility: The change should maintain compatibility with non-versioned models while adding support for versioned ones.
To ensure the changes work as intended:
These verification steps will help confirm that the
lookupByBaseNamemethod is properly implemented and that there are no remaining instances of the oldgetmethod being used withnodeMetaMap.Consider adding unit tests specifically for the
lookupByBaseNamemethod to ensure it correctly handles both versioned and non-versioned model names. This will help maintain the integrity of this critical functionality as the codebase evolves.src/commands/index.ts (1)
724-724: LGTM! Verify usage oflookupByBaseNameacross the codebase.The change from
nodeMetaMap.get(modelName)tonodeMetaMap.lookupByBaseName(modelName)aligns with the PR objective of supporting versioned dbt models. This modification allows the extension to work with model files that follow the versioning format (e.g.,<model_file_name>_v2.sql).To ensure consistency and identify any potential issues, let's verify the usage of
lookupByBaseNameacross the codebase:This will help us identify if there are any other instances where
nodeMetaMap.getmight need to be updated tolookupByBaseNamefor consistency.✅ Verification successful
Verified! The usage of
lookupByBaseNameis consistent across the codebase, and there are no remaining instances ofnodeMetaMap.get.This change aligns with the PR objective of supporting versioned dbt models and does not introduce any inconsistencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of lookupByBaseName and get echo "Occurrences of lookupByBaseName:" rg --type typescript "lookupByBaseName" -A 2 -B 2 echo "\nOccurrences of nodeMetaMap.get:" rg --type typescript "nodeMetaMap\.get" -A 2 -B 2Length of output: 323
Script:
#!/bin/bash # Search for other occurrences of lookupByBaseName and get in TypeScript files using file extensions echo "Occurrences of lookupByBaseName in .ts and .tsx files:" rg "lookupByBaseName" --glob "*.ts" --glob "*.tsx" -A 2 -B 2 echo "\nOccurrences of nodeMetaMap.get in .ts and .tsx files:" rg "nodeMetaMap\.get" --glob "*.ts" --glob "*.tsx" -A 2 -B 2Length of output: 8711
src/manifest/parsers/nodeParser.ts (1)
25-31: Handle missing entries inmodelMetadataLookupMapIn the
lookupByUniqueIdmethod, iflatestVersionUniqueIDis not found inmodelMetadataLookupMap, the function returnsundefined. Ensure that the calling code properly handlesundefinedvalues to prevent runtime errors.src/manifest/dbtProject.ts (3)
1169-1172: Consider usinglookupByUniqueIdinstead oflookupByBaseName.
1240-1243: Consider usinglookupByUniqueIdinstead oflookupByBaseName.
1296-1297: Consider usinglookupByUniqueIdinstead oflookupByBaseName.
| @@ -64,7 +64,7 @@ export class StaleModelColumnTest implements AltimateScanStep { | |||
| return; | |||
| } | |||
| const { nodeMetaMap } = projectEventMap; | |||
| for (const [key, value] of nodeMetaMap) { | |||
| for (const value of nodeMetaMap.nodes()) { | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of nodeMetaMap iteration across the codebase
The optimization to use for (const value of nodeMetaMap.nodes()) is only applied in test files. To ensure consistency and maximize performance benefits, please apply this iteration method to all relevant non-test TypeScript files.
- Action Required: Update non-test TypeScript files to use
nodeMetaMap.nodes()for iteration where applicable.
🔗 Analysis chain
Optimization approved, but clarification needed
The change from for (const [key, value] of nodeMetaMap) to for (const value of nodeMetaMap.nodes()) is a valid optimization. It eliminates the need for destructuring and focuses solely on the values, which is all that's needed in this loop.
However, I have a few points to address:
-
How does this change relate to the PR objective of supporting versioned dbt models? The connection isn't immediately clear.
-
Consider adding a comment explaining the reason for this change, e.g., "Directly iterate over node values for better performance".
-
It would be beneficial to verify if similar changes have been made consistently across the codebase where
nodeMetaMapis used.
To check for consistency, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of nodeMetaMap iteration
# Expected result: All occurrences should use the new .nodes() method
rg --type typescript 'for.*?nodeMetaMap' -g '!*.test.ts'
Length of output: 2314
Script:
#!/bin/bash
# Description: Check for consistent usage of nodeMetaMap iteration
# This script searches for 'for.*?nodeMetaMap.nodes()' in TypeScript files excluding test files
# Expected result: All occurrences should use the new .nodes() method
rg --type typescript 'for\s*\(\s*const\s+value\s+of\s+nodeMetaMap\.nodes\(\)\s*\)' -g '!*.test.ts'
Length of output: 135
Script:
#!/bin/bash
# Description: Check for consistent usage of nodeMetaMap iteration
# This script searches for 'for (const value of nodeMetaMap.nodes())' in TypeScript files excluding test files
# Expected result: All occurrences should use the new .nodes() method
rg 'for\s*\(\s*const\s+value\s+of\s+nodeMetaMap\.nodes\(\)\s*\)' -g '*.ts' -g '*.tsx' -g '!*.test.ts' -g '!*.test.tsx'
Length of output: 487
Script:
#!/bin/bash
# Description: Verify usage of nodeMetaMap.nodes() in non-test TypeScript files
# This script searches for 'for (const value of nodeMetaMap.nodes())' in TypeScript files excluding test files
# Expected result: No occurrences should be found if the change is only in test files
rg 'for\s*\(\s*const\s+value\s+of\s+nodeMetaMap\.nodes\(\)\s*\)' -g '*.ts' -g '*.tsx' -g '!*.test.ts' -g '!*.test.tsx'
Length of output: 487
| const url = nodeMetaMap.lookupByBaseName(nodeName)?.path!; | ||
| return new Analysis(nodeName, parentNodeName, url); |
There was a problem hiding this comment.
Add null check for analysis nodes
While the change to use nodeMetaMap.lookupByBaseName(nodeName) is good for supporting versioned models, there's a potential issue:
There's no null check before accessing the path property. This could lead to runtime errors if the lookup returns undefined. For consistency with the "model" and "seed" cases, and to improve robustness, consider adding a null check:
const analysis = nodeMetaMap.lookupByBaseName(nodeName);
const url = analysis?.path;
return analysis ? new Analysis(nodeName, parentNodeName, url ?? "") : undefined;This change will prevent potential runtime errors and make the code more consistent with the other cases.
| const url = nodeMetaMap.lookupByBaseName(nodeName)?.path!; | ||
| return new Snapshot(nodeName, parentNodeName, url); |
There was a problem hiding this comment.
Add null checks for snapshot and exposure nodes
The changes to use nodeMetaMap.lookupByBaseName(nodeName) in the "snapshot" and "exposure" cases are good for supporting versioned models. However, there's a potential issue:
There are no null checks before accessing the path property in both cases. This could lead to runtime errors if the lookup returns undefined. For consistency with the "model" and "seed" cases, and to improve robustness, consider adding null checks:
For the "snapshot" case:
const snapshot = nodeMetaMap.lookupByBaseName(nodeName);
const url = snapshot?.path;
return snapshot ? new Snapshot(nodeName, parentNodeName, url ?? "") : undefined;For the "exposure" case:
const exposure = nodeMetaMap.lookupByBaseName(nodeName);
const url = exposure?.path;
return exposure ? new Exposure(nodeName, parentNodeName, url ?? "") : undefined;These changes will prevent potential runtime errors and make the code more consistent across all cases.
Also applies to: 193-194
| export class NodeMetaMapImpl implements NodeMetaMap { | ||
| constructor( | ||
| private latestVersionLookupMap: Map<string, string> = new Map(), | ||
| private modelNameLookupMap: Map<string, string> = new Map(), | ||
| private modelMetadataLookupMap: Map<string, NodeMetaData> = new Map(), | ||
| ) {} | ||
|
|
||
| lookupByBaseName(modelBaseName: string): NodeMetaData | undefined { | ||
| const uniqueId = this.modelNameLookupMap.get(modelBaseName); | ||
| if (!uniqueId) { | ||
| return; | ||
| } | ||
| return this.lookupByUniqueId(uniqueId); | ||
| } | ||
|
|
||
| lookupByUniqueId(uniqueId: string): NodeMetaData | undefined { | ||
| const latestVersionUniqueID = this.latestVersionLookupMap.get(uniqueId); | ||
| if (latestVersionUniqueID) { | ||
| return this.modelMetadataLookupMap.get(latestVersionUniqueID); | ||
| } | ||
| return this.modelMetadataLookupMap.get(uniqueId); | ||
| } | ||
|
|
||
| nodes(): Iterable<NodeMetaData> { | ||
| return this.modelMetadataLookupMap.values(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unit tests for NodeMetaMapImpl class
The new NodeMetaMapImpl class introduces critical methods that handle node metadata lookups. Adding unit tests for lookupByBaseName, lookupByUniqueId, and nodes ensures they function correctly and handle edge cases.
| if (nodesMap === null || nodesMap === undefined) { | ||
| resolve(modelMetaMap); | ||
| resolve(new NodeMetaMapImpl(new Map(), new Map())); |
There was a problem hiding this comment.
Fix constructor call: Missing third argument
The constructor of NodeMetaMapImpl expects three arguments (latestVersionLookupMap, modelNameLookupMap, modelMetadataLookupMap), but only two are provided here. This will result in a runtime error.
Apply this diff to include the missing parameter:
- resolve(new NodeMetaMapImpl(new Map(), new Map()));
+ resolve(new NodeMetaMapImpl(new Map(), new Map(), new Map()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resolve(new NodeMetaMapImpl(new Map(), new Map())); | |
| resolve(new NodeMetaMapImpl(new Map(), new Map(), new Map())); |
| modelNameLookupMap.set(basename(fullPath, ".sql"), unique_id); | ||
| } |
There was a problem hiding this comment.
Handle file extensions correctly when using basename
The basename function's second parameter removes the specified suffix only if it matches exactly. If the file has an extension other than .sql, this might lead to incorrect model names.
Consider using path.parse to reliably extract the base name without extension:
-import { basename } from "path";
+import { basename, parse } from "path";
...
if (fullPath) {
- modelNameLookupMap.set(basename(fullPath, ".sql"), unique_id);
+ modelNameLookupMap.set(parse(fullPath).name, unique_id);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| modelNameLookupMap.set(basename(fullPath, ".sql"), unique_id); | |
| } | |
| modelNameLookupMap.set(parse(fullPath).name, unique_id); | |
| } | |
| ``` | |
| Note: This suggestion assumes that the `parse` function from the `path` module has been imported at the top of the file, as indicated in the diff snippet. If it hasn't been imported, you would need to add the import statement as well: | |
| ```typescript | |
| import { basename, parse } from "path"; |
| const parts = unique_id.split("."); | ||
| parts.pop(); | ||
| latestVersionLookupMap.set(parts.join("."), unique_id); | ||
| } |
There was a problem hiding this comment.
Ensure safe extraction of base unique ID
When extracting the base unique ID by splitting unique_id and removing the last element, there is a potential risk if unique_id does not contain any dots. This could lead to unexpected behavior or errors.
Apply this diff to safeguard against unique IDs without dots:
const parts = unique_id.split(".");
+ if (parts.length > 1) {
parts.pop();
+ }
latestVersionLookupMap.set(parts.join("."), unique_id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const parts = unique_id.split("."); | |
| parts.pop(); | |
| latestVersionLookupMap.set(parts.join("."), unique_id); | |
| } | |
| const parts = unique_id.split("."); | |
| if (parts.length > 1) { | |
| parts.pop(); | |
| } | |
| latestVersionLookupMap.set(parts.join("."), unique_id); |
| latest_version, | ||
| version, |
There was a problem hiding this comment.
Check for undefined latest_version and version properties
The properties latest_version and version might be undefined if they are missing in the nodesMap entries. Accessing them without validation could lead to runtime errors.
Apply this diff to include undefined checks:
if (version && latest_version && version === latest_version) {
const parts = unique_id.split(".");
parts.pop();
latestVersionLookupMap.set(parts.join("."), unique_id);
}
+ } else {
+ this.terminal.warn(
+ "NodeParser",
+ `Missing 'version' or 'latest_version' for node '${unique_id}'`
+ );
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Skipped PR review on ec42737 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
Generated with ❤️ by ellipsis.dev
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d97869e in 28 seconds
More details
- Looked at
24lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/webview_provider/newLineagePanel.ts:839
- Draft comment:
ReplacelookupByBaseNamewithlookupByUniqueIdto ensure correct model version support.
const node = nodeMetaMap.lookupByUniqueId(key);
- Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/webview_provider/newLineagePanel.ts:839
- Draft comment:
Utility functions should have a return type specified in their signature for better refactoring and readability. This applies togetHtml,getNonce,getUri, andreplaceInFile. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_Dp8hzd5rWXL979vx
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 1c6728d in 32 seconds
More details
- Looked at
48lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/webview_provider/sqlLineagePanel.ts:196
- Draft comment:
To support model versions, replacelookupByBaseNamewithlookupByUniqueIdfor consistency with other changes in the PR. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is redundant because the code change has already been made to uselookupByUniqueId. The comment does not provide any new information or action items for the PR author.
I might be missing the context of whether this comment is meant to explain the change rather than suggest it. However, the rules specify not to make purely informative comments.
The rules are clear that comments should only be kept if they indicate a necessary code change. Since the change is already made, the comment is not needed.
Remove the comment because the suggested change has already been implemented in the code.
2. src/manifest/dbtProject.ts:1295
- Draft comment:
UselookupByUniqueIdinstead oflookupByBaseNameto support model versions. This applies to other instances in this file as well. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_V3I7M44LN0gnNNrI
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 0e575ba in 21 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/webview_provider/sqlLineagePanel.ts:408
- Draft comment:
Ensure thatlookupByUniqueIdis used consistently across the codebase for model version support, as intended in the PR description. - Reason this comment was not posted:
Confidence changes required:50%
The change fromlookupByBaseNametolookupByUniqueIdin thegetColumnsmethod aligns with the PR description, which aims to support model versions by using unique identifiers.
2. src/webview_provider/sqlLineagePanel.ts:404
- Draft comment:
The variabletableNameis defined but not used. Consider removing it to clean up the code. - Reason this comment was not posted:
Confidence changes required:50%
The code useslookupByUniqueIdinstead oflookupByBaseNamefor node retrieval, which is correct as per the PR description. However, the variabletableNameis defined but not used, which is unnecessary.
Workflow ID: wflow_1tBf6v5bzlEVVsv4
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 804231b in 20 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/webview_provider/newLineagePanel.ts:850
- Draft comment:
Ensure thatnode.aliasis correctly set and available in all contexts wherecreateTableis used. This change aligns with the PR's intent to support model versions by using aliases. - Reason this comment was not posted:
Confidence changes required:50%
The change fromtabletonode.aliasin the return object ofcreateTablefunction is consistent with the PR description, which mentions updating model access methods to support model versions. This change seems to align with the intent to use the latest version or alias of the model.
2. src/webview_provider/newLineagePanel.ts:850
- Draft comment:
Ensure 'node.alias' is always defined to avoid potential issues when used as a label. - Reason this comment was not posted:
Confidence changes required:50%
The change in line 850 uses 'node.alias' for the label, which is a good practice for clarity. However, ensure that 'node.alias' is always defined to avoid potential issues.
Workflow ID: wflow_t9g0OFHY8gKuXFE7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 150059c in 29 seconds
More details
- Looked at
27lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. src/manifest/dbtProject.ts:1169
- Draft comment:
ReplacedlookupByBaseNamewithlookupByUniqueIdfor model version support, as per the PR description. - Reason this comment was not posted:
Comment did not seem useful.
2. src/manifest/dbtProject.ts:1241
- Draft comment:
ReplacedlookupByBaseNamewithlookupByUniqueIdfor model version support, as per the PR description. - Reason this comment was not posted:
Marked as duplicate.
3. src/manifest/dbtProject.ts:1236
- Draft comment:
Please specify a return type for thegetBulkCompiledSqlfunction to improve code readability and maintainability. - Reason this comment was not posted:
Marked as duplicate.
4. src/manifest/dbtProject.ts:1165
- Draft comment:
Please specify a return type for thegetNonEphemeralParentsfunction to improve code readability and maintainability. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_1KBmaJRi4egWd39W
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 229a023 in 25 seconds
More details
- Looked at
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/autocompletion_provider/modelAutocompletionProvider.ts:154
- Draft comment:
Ensure thatmodel.aliascorrectly supports model versions. Consider using a version-specific property if available. - Reason this comment was not posted:
Comment did not seem useful.
2. src/autocompletion_provider/modelAutocompletionProvider.ts:153
- Draft comment:
Do not abbreviate terms in comments. Use full words for clarity, e.g., 'support for model version' instead of 'support for model version'. This applies to the comment on line 7 as well. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_EJ2svxI6uKshYF3E
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b7046b8 in 21 seconds
More details
- Looked at
52lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/autocompletion_provider/modelAutocompletionProvider.ts:150
- Draft comment:
The TODO comment indicates that model version support is not yet implemented in the autocomplete feature. Ensure that model versions are correctly handled in themodelNamefield. - Reason this comment was not posted:
Comment did not seem useful.
2. src/autocompletion_provider/modelAutocompletionProvider.ts:140
- Draft comment:
Please provide a return type for thegetAutoCompleteItemsfunction to ensure easier refactoring and better readability. This applies to the function defined at line 180. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_ElRntb5IklZ6MMoc
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 124e672 in 24 seconds
More details
- Looked at
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/services/queryManifestService.ts:154
- Draft comment:
The TODO comment indicates that model versioning is not yet supported here. Since this PR is meant to address model versioning, this needs to be implemented before merging. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is directly related to the change in the diff, which is the modification of the return value in the 'getModelsInProject' function. The presence of a TODO suggests that the implementation is incomplete. If the PR is indeed meant to address model versioning, the comment is valid as it points out an incomplete implementation.
The comment assumes that the PR is meant to address model versioning, but this is not confirmed in the diff. Without additional context, it's speculative to say that this needs to be implemented before merging.
The presence of a TODO in the code is a strong indicator that the implementation is incomplete, which aligns with the comment's suggestion. However, without explicit confirmation that the PR is meant to address model versioning, the comment could be speculative.
The comment should be kept as it highlights an incomplete implementation related to the TODO, which is directly connected to the code change in the diff.
2. src/services/queryManifestService.ts:154
- Draft comment:
Please specify a return type for thegetModelsInProjectfunction to improve code readability and maintainability. This issue is also present in other functions likegetSourcesInProject. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_hkPIKnk5PjJoNCHB
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d76036f in 18 seconds
More details
- Looked at
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/manifest/parsers/graphParser.ts:164
- Draft comment:
Ensure thatlookupByUniqueId()is used consistently for model version support, as per the PR description. The current usage here is correct. - Reason this comment was not posted:
Confidence changes required:20%
The PR description indicates thatlookupByUniqueId()should be used for model version support. The current implementation inmapToNodemethod already useslookupByUniqueId()for models and seeds, which aligns with the PR description. However, thecreateGraphMetaMapmethod should also be checked for correct usage oflookupByUniqueId()orlookupByBaseName()as per the PR description.
2. src/manifest/parsers/graphParser.ts:164
- Draft comment:
Please specify a return type for themapToNodefunction to improve code readability and maintainability. This applies to other utility functions as well. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_unh1njmv9ESPisL5
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Overview
Problem
Fixes #709
Solution
Describe the implemented solution. Add external references if needed.
Screenshot/Demo
A picture is worth a thousand words. Please highlight the changes if applicable.
How to test
Checklist
README.mdupdated and added information about my changeImportant
Add support for model versions by updating model access methods across the codebase to ensure the latest version is used.
nodeMetaMap.get()withnodeMetaMap.lookupByBaseName()inindex.ts,sqlToModel.ts, andvalidateSql.tsto support model versions.nodeMetaMap.entries()andnodeMetaMap.values()withnodeMetaMap.nodes()inmodelAutocompletionProvider.ts,sqlToModel.ts, andvalidateSql.ts.NodeMetaMapImplclass innodeParser.tsto implementNodeMetaMapinterface with methodslookupByBaseName(),lookupByUniqueId(), andnodes().NodeMetaMapinterface indomain.tsto include new methods for handling model versions.getNonEphemeralParents()andgetBulkCompiledSql()indbtProject.tsto uselookupByBaseName()for model version support.createGraphMetaMap()ingraphParser.tsto uselookupByUniqueId()for model version support.This description was created by
for d76036f. It will automatically update as commits are pushed.
Summary by CodeRabbit
Release Notes
Improvements
SQLLineagePanelby excluding unnecessary elements from the event object.getModelsInProjectmethod to provide clearer return types and simplify logic for model names retrieval.Bug Fixes