feat(kotlin): update language server to 262.9593.0 - #1811
Conversation
a83fbce to
cf77047
Compare
|
@opcode81 is using the new mechanism you introduced for managing LS dependencies appropriate here? |
|
I did some other work here beside just updating the kotlin LS, but i could keep only those changes and then the new LS dependency management could be run after to update kotlin ls |
MischaPanch
left a comment
MischaPanch
left a comment
There was a problem hiding this comment.
I will let @opcode81 review, so I just had a brief look and some comments. This bypasses existing download mechanisms - both old and new - thus increasing code complexity. If possible, such bypassing should be avoided
| "osx-x64": (".sit", "zip", ("kotlin-server-{version}", "bin", "intellij-server")), | ||
| "osx-arm64": ("-aarch64.sit", "zip", ("kotlin-server-{version}", "bin", "intellij-server")), | ||
| } | ||
| KOTLIN_SERVER_LAUNCHER_NAMES = frozenset({"intellij-server", "intellij-server.exe"}) |
There was a problem hiding this comment.
one name should be enough, we can condition on the OS
| url=f"https://download-cdn.jetbrains.com/kotlin-lsp/{version}/kotlin-lsp-{version}-{kotlin_suffix}.zip", | ||
| archive_type="zip", | ||
| launcher_parts=("kotlin-lsp.cmd",) if platform_id.is_windows() else ("kotlin-lsp.sh",), | ||
| sha256=INITIAL_KOTLIN_LSP_SHA256_BY_SUFFIX.get(kotlin_suffix) if version == INITIAL_KOTLIN_LSP_VERSION else None, |
There was a problem hiding this comment.
why is there no sha on this path? Generally, the branching should be documented
| raise ValueError(f"Kotlin LSP version must contain only dot-separated integers: {version!r}") from exc | ||
|
|
||
|
|
||
| def _uses_kotlin_server_packaging(version: str) -> bool: |
There was a problem hiding this comment.
I know this is just an isolated small thing, but still I'm not a fan of these functions. They could be inlined, they seem to only exist to support tests, and tests should generally not import private functions unless there is a very good reason. I also doubt that the tests add too much value giving the amount of mocking going on there.
Overall, I think the code selecting the download path can be shortened and explained a bit better.
|
i didn't think about it in previous comment, but now i updated the Kotlin provider to use DownloadedDependency from #1799. Addressed the three review points. Launcher detection is now OS-specific, the pinned initial and current artifacts use the central checksum database and arbitrary custom versions explicitly remain unverified, and the download-path selection is consolidated and documented without the top-level private helpers. |
…9593.0 Conflicts: CHANGELOG.md src/solidlsp/resources/downloaded_dependency_hashes.json
|
Thanks! |
3 participants
Summary
261.13587.0to262.9593.0..sit,.tar.gz, and.win.ziparchives, bundled JBR launchers, and Windows ARM64 builds.JetBrains still labels Kotlin LSP pre-alpha and does not publish a formal stable channel. This uses the newest published release while retaining Serena's existing CI quarantine for the resource-sensitive live Kotlin tests.
Validation
uv run pytest -vv test/solidlsp/kotlin/test_kotlin_dependency_provider.py test/solidlsp/kotlin/test_kotlin_startup.py(17 passed)CI=true(17 passed, confirming the new regression tests are not quarantined)GRADLE_USER_HOME=... uv run pytest -vv test/solidlsp/kotlin/test_kotlin_basic.py -m kotlinon Python 3.13.14 (4 passed)uv run poe lintuv run poe type-checkgit diff --checkChecklist
CONTRIBUTING.mdregarding the scope of PRs.CHANGELOG.md, which concisely describes the change.