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

add LSP-rome #1

Merged
merged 19 commits into from
Jun 18, 2023
Merged

add LSP-rome #1

merged 19 commits into from
Jun 18, 2023

Conversation

rchl
Copy link
Member

@rchl rchl commented Dec 22, 2022

TODO

  • Lookup rome binary in the workspace
  • Update schema with some extra snippets
  • Update Readme with more info

@predragnikolic
Copy link
Member

predragnikolic commented Dec 24, 2022

notes:

  • it would be good if we have json schema support in rome.json files. The schema should ideally exist here... but that is not the case. For the time being users could specify "$schema": "./node_modules/rome/configuration_schema.json", as mentioned in their docs.
  • Add source.organizeImports.rome code action on save. The code action works in VS Code, but not in ST.

1. rome.json options not respected.

I have the following index.js file:

function x() {
let d = 21;
}

I have the following rome.json file:

{
  "$schema": "./node_modules/rome/configuration_schema.json",
  "formatter": {
    "enabled": true,
    "formatWithErrors": false,
    "indentStyle": "space",
    "indentSize": 2,
    "lineWidth": 80,
    "ignore": []
  }
}

When i run format file, this request is send:

:: --> LSP-rome textDocument/formatting(68): {'workDoneToken': 'wd68', 'options': {'trimTrailingWhitespace': True, 'insertFinalNewline': False, 'tabSize': 4, 'trimFinalNewlines': False, 'insertSpaces': False}, 'textDocument': {'uri': 'file:///home/predragnikolic/Documents/sandbox/rome/index.js'}}
:: <<< LSP-rome 68: [{'range': {'start': {'character': 0, 'line': 0}, 'end': {'character': 0, 'line': 5}}, 'newText': 'function x() {\n\tlet d = 21;\n}\n'}]

and the file gets formatted with tabs although I specified spaces.
the file uses tab size 4, although I specified 2. So the file gets formatted like this:

function x() {
	let d = 21;
}

But I expect this:

function x() {
  let d = 21;
}

2, At some point Rome shows an error for a valid file...

This looks like a server issue. I don't know how I managed to trigger it. I guess there is nothing we could do about it, except being aware that it can happen. EDIT: you already opened an issue rome/tools#4094 :)
image

@rchl
Copy link
Member Author

rchl commented Dec 24, 2022

it would be good if we have json schema support in rome.json files.

It's already present in sublime-package.json.

Add source.organizeImports.rome code action on save.

Probably because server doesn't advertise support for it. We should investigate whether we should report this in Rome or whether we should align our LSP logic.

  1. rome.json options not respected.

Will check it after adding support for using workspace server.

2, At some point Rome shows an error for a valid file...

Yep, already reported.

@predragnikolic
Copy link
Member

It's already present in sublime-package.json.

Ok, it works. I don't know what happened today. It seemed like it didn't work.

@predragnikolic
Copy link
Member

The server relies on format on save.
If I enable format on save in LSP for ST, it will enable it for all servers, which is not what I would want.

I really wish that they went with code actions on save instead of format on save.

@rchl
Copy link
Member Author

rchl commented Jan 21, 2023

I can't reproduce "1". My rome.json is at the root of the workspace folder.

LSP-rome.sublime-settings Outdated Show resolved Hide resolved
@rchl
Copy link
Member Author

rchl commented Jan 29, 2023

I've seen on their Discord channel that the project might be in a bit of a trouble financially and a couple of developers left due to that. Might not have a bright future.

@thomasjiangcy
Copy link

Thanks for the work! Any chance this will be merged/released any time soon?

@rchl
Copy link
Member Author

rchl commented Jun 18, 2023

I can merge it.

I've noticed that the server might randomly quit because of what I think are some issues in flushing the stdout output. But it's something that would have to be reported to Rome if it keeps happening.

@rchl
Copy link
Member Author

rchl commented Jun 18, 2023

I've added LSP-rome: Organize Imports command to Command Palette.
Server doesn't advertise support for the source.organizeImports.rome code action but it's possible to invoke it as long as it's enabled in rome.json.

Also changed default value of rome.requireConfiguration to true to match VSCode extension and because otherwise it reports a lot of issues in projects that don't really expect rome checks.

Also updated to latest version of the server - 12.1.3

@rchl rchl merged commit 938ab3c into main Jun 18, 2023
@rchl rchl deleted the feat/add branch June 18, 2023 14:40
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