Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the lsp-mssql package to use a newer version of the SQL Tools Service from GitHub releases instead of the Microsoft download server, and modernizes the codebase for compatibility with newer versions of dependencies.
- Updates the server download URL to use GitHub releases (version 5.0.20250910.2)
- Migrates from .NET Core 2.2 to .NET 8.0 platform targets
- Replaces deprecated
org-show-allwithorg-fold-show-all - Refactors the object explorer to use
lsp-treemacs-renderinstead of manual buffer setup
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| (let* ((result (cond ((eq system-type 'darwin) "osx-arm64-net8.0.tar.gz") | ||
| ((eq system-type 'gnu/linux) "linux-x64-net8.0.tar.gz") | ||
| ((eq system-type 'windows-nt) "win-x64-net8.0.zip") | ||
| (t (error (format "Unsupported system: %s" system-type))))) |
There was a problem hiding this comment.
The hardcoded ARM64 architecture for macOS may not work on Intel-based Macs. Consider detecting the actual architecture or providing both x64 and arm64 options for macOS.
| (let* ((result (cond ((eq system-type 'darwin) "osx-arm64-net8.0.tar.gz") | |
| ((eq system-type 'gnu/linux) "linux-x64-net8.0.tar.gz") | |
| ((eq system-type 'windows-nt) "win-x64-net8.0.zip") | |
| (t (error (format "Unsupported system: %s" system-type))))) | |
| (let* ((result (cond | |
| ((eq system-type 'darwin) | |
| (if (or (string-match "arm64" system-configuration) | |
| (string-match "aarch64" system-configuration)) | |
| "osx-arm64-net8.0.tar.gz" | |
| "osx-x64-net8.0.tar.gz")) | |
| ((eq system-type 'gnu/linux) "linux-x64-net8.0.tar.gz") | |
| ((eq system-type 'windows-nt) "win-x64-net8.0.zip") | |
| (t (error (format "Unsupported system: %s" system-type))))) |
There was a problem hiding this comment.
This seems like a valid concern, but I have no way to verify the values of system-configuration on a mac as I don't have one or anyone who could check. The variable does exist though and on my linux system it has value "x86_64-pc-linux-gnu" so it seems legit.
There was a problem hiding this comment.
I think it's pretty safe to do it this way. I have several packages that do this, and I have no issues with them.
Examples:
- https://github.com/emacs-tree-sitter/tree-sitter-langs/blob/698a17da1f8e2bd34858ce46c727f492c69d123b/tree-sitter-langs-build.el#L274-L297
- https://github.com/jcs-elpa/flx-rs/blob/b47b9e9b6f17f21d46bbd0d28f408006d6ac2dd4/flx-rs.el#L74-L84
I'm running both packages on macOS M3.
| (lsp-treemacs-render tree title 0 | ||
| "*SQL Object explorer*" nil) | ||
| (with-current-buffer "*SQL Object explorer*" | ||
| (display-buffer-in-side-window (current-buffer) '((side . right))) | ||
| (lsp-mssql-object-explorer-mode))) |
There was a problem hiding this comment.
The indentation uses tabs instead of spaces, which is inconsistent with Emacs Lisp conventions. Use consistent space-based indentation.
| ;; - (with-current-buffer (get-buffer-create "*SQL Object explorer*") | ||
| ;; - (lsp-treemacs-initialize) | ||
| ;; - (setq-local lsp-treemacs-tree tree) | ||
| ;; - (setq-local face-remapping-alist '((button . default))) | ||
| ;; - (lsp-treemacs-generic-refresh);; - | ||
| ;; - (setq-local mode-name title) | ||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Commented-out code should be removed rather than left in the source. If this code might be needed for reference, consider documenting the migration in a commit message instead.
| ;; - (with-current-buffer (get-buffer-create "*SQL Object explorer*") | |
| ;; - (lsp-treemacs-initialize) | |
| ;; - (setq-local lsp-treemacs-tree tree) | |
| ;; - (setq-local face-remapping-alist '((button . default))) | |
| ;; - (lsp-treemacs-generic-refresh);; - | |
| ;; - (setq-local mode-name title) |
3 participants
Fixes issue 22 - #22