scalameta / metals

Scala language server with rich IDE features 🚀
https://scalameta.org/metals/
Apache License 2.0
2.1k stars 333 forks source link

Range formatting is not working in Emacs #1649

Closed chessman closed 4 years ago

chessman commented 4 years ago

Describe the bug When I select a few lines and execute lsp-format-region command nothing happens.

To Reproduce Steps to reproduce the behavior:

  1. Select few lines using visual selection.
  2. Execute lsp-format-region.

Expected behavior The lines are formatted.

LSP log

[Trace - 05:38:01 PM] Sending request 'textDocument/rangeFormatting - (2273)'.
Params: {
  "textDocument": {
    "uri": "file:///home/ea/src/<project>/src/main/scala/<path>/<file>.scala"
  },
  "options": {
    "tabSize": 8,
    "insertSpaces": true
  },
  "range": {
    "start": {
      "line": 81,
      "character": 0
    },
    "end": {
      "line": 87,
      "character": 0
    }
  }
}

[Trace - 05:38:01 PM] Received response 'textDocument/rangeFormatting - (2273)' in 4ms.
Result: []

Installation:

Additional context The whole file formatting command lsp-format-buffer is working correctly.

mlachkar commented 4 years ago

Hello, In fact, range formatting only knows how to format multiline strings. It's not yet implemented for any range of lines. Thanks for reporting!

tgodzik commented 4 years ago

Hello, In fact, range formatting only knows how to format multiline strings. It's not yet implemented for any range of lines.

Just to add, we use scalafmt for code formatting, which doesn't support formatting selection, so we don't forsee it being supported soon. We do support as @mlachkar said formatting for selections inside a multiline string.

tgodzik commented 4 years ago

We could support other formatting tools, but for that we can open a feature request.

danking commented 4 years ago

I ran into this today as well. Metals claims to support range formatting, according to LSP:

       |-[X] documentRangeFormattingProvider: t

There's no option in M-x customize-group RET lsp-metals to disable metals as a range formatting provider.

If I disable LSP range formatting entirely (which I'd rather not do, as it impacts every language I use), then M-x indent-region defaults to indent-region-line-by-line which works perfectly.

Is it possible to make documentRangeFormattingProvider configurable so that users can at least disable this behavior? Moreover, I urge you to consider setting the default value to nil, not t. The vast majority of things that I want to range format are blocks of code, not multi-line strings.

tgodzik commented 4 years ago

@danking Thanks for reporting. Is there any particular issue that the range formatting is currently causing? We will not support range formatting for code for the forseeable future, because scalafmt, which we use, only supports the entire file. Because of that range formatting only works on multiline strings to do things like change:

val str = """| 
               |
 |
|""".stripMargin

into

val str = """| 
             |
             |
             |""".stripMargin

This should not cause any issues and I think this behaviour is perfectly acceptable. It especially help with formatting on paste in VS Code.

I am not too familiar with Emacs, but if it causes issues there please do open up an issue.

danking commented 4 years ago

The particular issue is that, in Emacs at least, there is no command for "range format this selected multi-line string". There is a command for "range format". Emacs users expect that to fix the formatting of the selected lines of code. Metals' range formatter only does a subset of that (formatting multi-line strings), notably it does not format all code. When an LSP implementation specifies documentRangeFormattingProvider: nil (i.e. no support), Emacs falls back to a generic, best-effort, c-style formatter. This formatter works well for Scala.

It's a bit like ordering a pizza and receiving cooked dough. It's certainly part of what I ordered, but I'd prefer you just told me you didn't make pizzas.

Anyway, here's a PR that implements what I need to get productive again: https://github.com/scalameta/metals/pull/1717 I had a few issues that hopefully you can help me resolve. Thank you!

dangerousben commented 4 years ago

@danking what was the outcome of this from the point of view of an emacs user who wants working region indentation?

I had planned to attack the problem from the emacs side by providing an indent-region-function like this:

(defun lsp-metals--indent-region (start end)
  (indent-region-line-by-line start end)
  (lsp-format-region (start end)))

ie, use the local indentation then see if metals has anything to add.

danking commented 4 years ago

Oh neat, that’s a great idea. Metals is also configurable now. I’ve been using a source build for a while now so I don’t know if my change has been released yet.