Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Pkl language server #3440

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: Pkl language server #3440

wants to merge 2 commits into from

Conversation

deverts
Copy link

@deverts deverts commented Nov 17, 2024

Here's a language server config for Pkl.

The stars on the LSP repo itself is below the recommended (it's 28 as of writing), but it's fairly new (first release 2 weeks back, October 10) and the main language is at >10k.

It looks as though it's actively developed by the maintainers of the language, and I'd assume development will continue. I've tested the current release locally, and it works as expected.

Let me know if you think it's too early, happy to close this for now and keep a fork around until they make more progress.

LSP for the Pkl configuration language
@deverts deverts requested a review from glepnir as a code owner November 17, 2024 19:42
Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that preamble. Sounds fine to me :)


return {
default_config = {
cmd = { 'java', '-jar', 'path/to/pkl-lsp.jar' },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is path/to/...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path depends on the user, the path here is just a placeholder.

Updated to match how similarly-configured LSPs are set up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the point of this config if it requires the user to manually configure the path?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at a couple other java-based LSPs for inspiration (nextflow_ls, groovyls if you're curious).

I'm happy to remove cmd from default_config, especially since the doc covers it. Let me know what you think fits best :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants