Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes the hard dependency on the "RandomFractalsInc.vscode-data-table" extension and implements dynamic detection to suggest installation when table data is rendered. The change allows the extension to function independently while still promoting better table visualization.
- Removed the extension dependency from package.json
- Added dynamic detection of the data table extension with installation suggestion
- Updated version to 2.8.3
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vscode/treeviews/connections/DatabricksConnectionManager.ts | Added methods to detect if data table extension is installed |
| src/vscode/notebook/DatabricksKernel.ts | Added logic to suggest data table extension installation when rendering tables |
| package.json | Removed hard dependency and updated version |
| CHANGELOG.md | Added release notes for v2.8.3 |
| .vscode/launch.json | Enabled extension disabling for development |
| .github/workflows/create_release_and_publish.yaml | Added conditional check for OpenVSX publishing |
Comments suppressed due to low confidence (1)
src/vscode/notebook/DatabricksKernel.ts:1
- [nitpick] Consider using
constinstead ofletfordtrHtmlanddtrOutputsince these variables are not reassigned.
import * as vscode from 'vscode';
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (!dataTableRenderers) { | ||
| this._isDataTableRenderersInstalled = false; | ||
| } | ||
| else { | ||
| this._isDataTableRenderersInstalled = true; | ||
| } |
There was a problem hiding this comment.
[nitpick] The if-else statement can be simplified using a ternary operator or direct assignment: this._isDataTableRenderersInstalled = !!dataTableRenderers;
| if (!dataTableRenderers) { | |
| this._isDataTableRenderersInstalled = false; | |
| } | |
| else { | |
| this._isDataTableRenderersInstalled = true; | |
| } | |
| this._isDataTableRenderersInstalled = !!dataTableRenderers; |
| ]) | ||
|
|
||
| execution.appendOutput(dtrOutput); | ||
| ThisExtension.ConnectionManager.resetIsDataTableRenderersInstalled(); |
There was a problem hiding this comment.
Calling resetIsDataTableRenderersInstalled() after checking the extension status seems counterintuitive. This will force the extension detection to run again on the next check, which may not be the intended behavior if the extension status hasn't changed.
| ThisExtension.ConnectionManager.resetIsDataTableRenderersInstalled(); |
No description provided.