fix: [AI-1773] improve sqlfmt discovery for uv tool install locations#1852
Open
ralphstodomingo wants to merge 5 commits intomasterfrom
Open
fix: [AI-1773] improve sqlfmt discovery for uv tool install locations#1852ralphstodomingo wants to merge 5 commits intomasterfrom
ralphstodomingo wants to merge 5 commits intomasterfrom
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCaches the resolved sqlfmt binary per configured Python interpreter and replaces a single Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as VS Code Extension
participant Provider as DbtDocumentFormattingEditProvider
participant PyFS as Python Interpreter FS
participant Env as Environment (PATH / PIPX_BIN_DIR / UV_TOOL_BIN_DIR)
participant UV as uv CLI
participant Which as system which
Editor->>Provider: Request sqlfmt path
Provider->>Provider: return cached path if valid
Provider->>PyFS: check for sqlfmt next to configured Python
alt found
PyFS-->>Provider: return path
else not found
Provider->>Env: probe known uv/pipx bin dirs & env overrides
alt found
Env-->>Provider: return path
else not found
Provider->>UV: run "uv tool dir" (async, 3s timeout)
alt uv returns dir
UV-->>Provider: tool dir
Provider->>Provider: construct shandy-sqlfmt candidate path(s) and check
alt found
Provider-->>Editor: resolved path
else
Provider->>Which: which("sqlfmt")
Which-->>Provider: path or undefined
Provider-->>Editor: resolved path or undefined + enhanced error message
end
else uv fails/timeout
Provider->>Which: which("sqlfmt")
Which-->>Provider: path or undefined
Provider-->>Editor: resolved path or undefined + enhanced error message
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| // Windows: uv puts tools in %APPDATA%\uv\data\tools\sqlfmt\... | ||
| const appData = process.env.APPDATA; | ||
| if (appData) { | ||
| candidates.push( |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Replace blocking execSync with async exec for uv tool dir lookup - Cache discovered path after first successful resolution - Respect UV_TOOL_BIN_DIR and PIPX_BIN_DIR env var overrides - Use correct uv tool directory name (shandy-sqlfmt, not sqlfmt) - Normalize exe name (sqlfmt vs sqlfmt.exe) based on platform Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/document_formatting_edit_provider/dbtDocumentFormattingEditProvider.ts`:
- Around line 115-119: The venv-adjacent lookup only checks for "sqlfmt" and
misses Windows executables; update the block that builds candidatePath (using
pythonPath and path.dirname(pythonPath)) to check for the Windows binary as
well—e.g. check for both path.join(path.dirname(pythonPath), "sqlfmt") and
path.join(path.dirname(pythonPath), "sqlfmt.exe") (or choose the
platform-specific name via process.platform === 'win32') and return the first
existing candidate; adjust the fs.existsSync checks accordingly so the function
(and symbol candidatePath) discovers sqlfmt on Windows venvs.
- Around line 149-169: The Windows/Linux candidate path push block in
dbtDocumentFormattingEditProvider.ts (the if (process.platform === "win32") {
... } else { ... } section that calls candidates.push and path.join) is
formatted in a way that violates the repo's ESLint/Prettier rules; reformat this
block to satisfy the project's formatter (wrap/indent arguments consistently and
remove the current line-wrapping style), then run the project's formatting
script (e.g., the repository's eslint/prettier or yarn/npm format command) to
apply the canonical formatting so ESLint/Prettier no longer flags the file.
- Around line 102-104: Update the error message string in
dbtDocumentFormattingEditProvider (the message built where it says "Could not
run sqlfmt...") to reference the correct PyPI package name "shandy-sqlfmt" and
the recommended install command (e.g. `uv tool install
"shandy-sqlfmt[jinjafmt]"`), replacing the incorrect `sqlfmt` reference while
keeping guidance about setting `dbt.sqlFmtPath` and restarting VS Code; locate
the message construction in the formatting provider code and update the text
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f59f51f5-8baf-4871-82a2-787c7d002fcf
📒 Files selected for processing (1)
src/document_formatting_edit_provider/dbtDocumentFormattingEditProvider.ts
src/document_formatting_edit_provider/dbtDocumentFormattingEditProvider.ts
Outdated
Show resolved
Hide resolved
src/document_formatting_edit_provider/dbtDocumentFormattingEditProvider.ts
Show resolved
Hide resolved
src/document_formatting_edit_provider/dbtDocumentFormattingEditProvider.ts
Outdated
Show resolved
Hide resolved
If the cached binary is deleted (e.g. uv tool uninstall), re-run discovery instead of silently failing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment on lines
+129
to
+132
| const result = await this.discoverSqlFmtPath(); | ||
| this.cachedSqlFmtPath = result; | ||
| this.sqlFmtPathResolved = true; | ||
| return result; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| window.showErrorMessage( | ||
| extendErrorWithSupportLinks( | ||
| "Could not run sqlfmt. Did you install sqlfmt? Detailed error: " + | ||
| "Could not run sqlfmt. If sqlfmt is installed (e.g. via `uv tool install sqlfmt` or `pipx install sqlfmt`), " + |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reference `shandy-sqlfmt[jinjafmt]` instead of bare `sqlfmt` in the install command suggestions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/document_formatting_edit_provider/dbtDocumentFormattingEditProvider.ts`:
- Around line 30-32: The cached binary path
(cachedSqlFmtPath/sqlFmtPathResolved) is not tied to the current Python
interpreter so switching interpreters can return a stale sqlfmt; modify
findSqlFmtPath() (and the other similar block around the second occurrence) to
include the interpreter context when caching — e.g., store the
lastSeenPythonPath (or incorporate this.pythonEnvironment.pythonPath) alongside
cachedSqlFmtPath and only reuse the cache if the stored interpreter path equals
the current this.pythonEnvironment.pythonPath; otherwise clear/refresh the cache
and call discoverSqlFmtPath() to resolve a new path.
- Around line 27-29: The class declaration wrapping violates Prettier; update
the declaration for DbtDocumentFormattingEditProvider so it adheres to project
formatting rules by placing the implements clause inline with the class
declaration (i.e., make the DbtDocumentFormattingEditProvider implements
DocumentFormattingEditProvider declaration a single line) ensuring consistent
spacing and braces to satisfy Prettier/ESLint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af6f4af7-6236-407f-a25c-fbbe253f654a
📒 Files selected for processing (1)
src/document_formatting_edit_provider/dbtDocumentFormattingEditProvider.ts
src/document_formatting_edit_provider/dbtDocumentFormattingEditProvider.ts
Outdated
Show resolved
Hide resolved
src/document_formatting_edit_provider/dbtDocumentFormattingEditProvider.ts
Show resolved
Hide resolved
…rmatting Cache invalidates when Python interpreter changes (e.g. switching venvs), ensuring the correct sqlfmt binary is discovered for the active environment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
~/.local/bin/, Windows uv/pipx paths)UV_TOOL_BIN_DIRandPIPX_BIN_DIRenvironment variable overridesexecinstead of blockingexecSyncforuv tool dirlookupshandy-sqlfmt, notsqlfmt) for tool directory pathsContext
Fixes #1773
When sqlfmt is installed via
uv tool install "shandy-sqlfmt[jinjafmt]", the extension couldn't find it. The binary is placed at~/.local/bin/sqlfmtbut VS Code's process PATH is frozen at launch time.Discovery order:
UV_TOOL_BIN_DIR/PIPX_BIN_DIRenv var overrides (new)~/.local/bin/sqlfmt— default for uv/pipx on Linux/Mac (new)uv tool dirasync discovery for custom locations (new, non-blocking)which(existing)Risk mitigations:
uv tool diruses async exec (non-blocking) with 3s timeoutfs.existsSyncvalidation — cache auto-invalidates if binary is removedUV_TOOL_BIN_DIRandPIPX_BIN_DIRrespected for users with custom tool locationsshandy-sqlfmtused in uv tool directory paths~/.local/bin/sqlfmtsymlink still finds the binary viauv tool dirfallbackTest plan
uv tool install— Format Document worksuv tool uninstall shandy-sqlfmt— formatting fails with improved error message🤖 Generated with Claude Code
Summary by CodeRabbit