-
-
Notifications
You must be signed in to change notification settings - Fork 424
Add package for marko-language-server #8350
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package folder name should also be changed to marko-language-sever
and not marko-js
packages/marko-js/package.yaml
Outdated
- LSP | ||
|
||
source: | ||
id: pkg:npm/%40marko/[email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id: pkg:npm/%40marko/[email protected] | |
id: pkg:npm/@marko/[email protected] | |
schemas: | |
lsp: vscode:https://raw.githubusercontent.com/marko-js/language-server/@marko/language-server@{{version}}/packages/vscode/package.json | |
bin: | |
marko-language-server: npm:@marko/language-server |
We should include the LSP schema with the package specification as well as the binary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should include the LSP schema with the package specification as well as the binary
True, but the %40 can stay, has to do with encoding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id: pkg:npm/%40marko/[email protected] | |
id: pkg:npm/%40marko/[email protected] | |
schemas: | |
lsp: vscode:https://raw.githubusercontent.com/marko-js/language-server/@marko/language-server@{{version}}/packages/vscode/package.json | |
bin: | |
marko-language-server: npm:%40marko/language-server |
Like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the main thing is that the %40
is not necessary here. There are many other packages that just use @
in the npm
string and they seem to work. So it seems better to use @
for readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks both for the review. It's been a while since I first opened this PR so it's hard to remember clearly, but I believe I was having trouble getting this working without the %40
encoding which is why I included it. I'm on a Windows machine so it might have been something to do with that? Either way I've updated this PR to:
- change the directory to
marko-language-server
- add the schema and bin to the config file
packages/marko-js/package.yaml
Outdated
- LSP | ||
|
||
source: | ||
id: pkg:npm/%40marko/[email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id: pkg:npm/%40marko/[email protected] | |
id: pkg:npm/%40marko/[email protected] | |
schemas: | |
lsp: vscode:https://raw.githubusercontent.com/marko-js/language-server/@marko/language-server@{{version}}/packages/vscode/package.json | |
bin: | |
marko-language-server: npm:%40marko/language-server |
Like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Describe your changes
Add language server for Marko-js
Issue ticket number and link
Checklist before requesting a review
Screenshots