Skip to content

Add format debounce and improve debounce time setup #141

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

Merged
merged 6 commits into from
Jul 7, 2021
Merged

Add format debounce and improve debounce time setup #141

merged 6 commits into from
Jul 7, 2021

Conversation

ray-x
Copy link
Contributor

@ray-x ray-x commented Jun 21, 2021

The PR

  1. Trying to improve the lintDebounce setup. Currently it only accepts int and it likely to mistake the time unit to ms. The golang Duration unit is ns. That means the setup need to use 500000000 to debounce 500ms. I did a quick grep of setups (dotfiles) in github. Most people are setting this field to 500, I believe they mean to set up debounce to 500ms. The changes allow user setup the debounce to "500ms" or "0.5s".

  2. Add format debounce. I saw random failures when multiple format tools were configured and will trigger lsp format multiple times. This sometime result in an incorrect result.

@mattn
Copy link
Owner

mattn commented Jul 1, 2021

Please rebase master branch


"github.com/sourcegraph/jsonrpc2"
)

func (h *langHandler) handleTextDocumentFormatting(ctx context.Context, conn *jsonrpc2.Conn, req *jsonrpc2.Request) (result interface{}, err error) {
func (h *langHandler) handleTextDocumentFormatting(
Copy link
Owner

Choose a reason for hiding this comment

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

I don't prefer this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was reverted.

@mattn
Copy link
Owner

mattn commented Jul 1, 2021

Currently it only accepts int

Really?

@ray-x
Copy link
Contributor Author

ray-x commented Jul 2, 2021

Hi, Matt
The branch was updated.
Thanks!

@mattn
Copy link
Owner

mattn commented Jul 6, 2021

I think that Duration is not needed.

@ray-x
Copy link
Contributor Author

ray-x commented Jul 6, 2021

The change to Duration is necessary

  1. It allows users set the interval in the format of 1s, 0.5s, 30ms. This is more explicit.
  2. Golang interval is nanosecond based. That is foreign to the user from lua/vimscript world where the setup is millisecond based. By searching the github I found most of users set the interval to values 200. But it won't work as that is 0.0000002ms. So it would be better to specify the setup in format of 200ms. And Duration still allow set value in float format for backward capacity. Without this change, the setup of 200ms will fail to parse and fall back to 0.
    There are discussions all: decide what can implement TextMarshaler/TextUnmarshaler golang/go#10275

@mattn
Copy link
Owner

mattn commented Jul 7, 2021

@ray-x
Copy link
Contributor Author

ray-x commented Jul 7, 2021

I did not test yaml setup for efm-languageserver
The reason for the change is neovim is using json to communicate with efm. that is not covered by go-yaml package.

@mattn
Copy link
Owner

mattn commented Jul 7, 2021

Could you please show me your neovim configuration for the efm-langserver?

@ray-x
Copy link
Contributor Author

ray-x commented Jul 7, 2021

  lspconfig.efm.setup {
    flags = {debounce_text_changes = 1000},
    cmd = {'efm-langserver', '-loglevel', '2', '-logfile', '/tmp/efm.log'}, 
    init_options = {documentFormatting = true},
    on_attach = function(client)
      client.resolved_capabilities.document_formatting = true
      client.resolved_capabilities.goto_definition = false
      client.resolved_capabilities.code_action = nil
      vim.cmd([[autocmd BufWritePre <buffer> lua vim.lsp.buf.formatting()]])
    end,
    filetypes = {
      "javascript", "javascriptreact", 'typescript', 'typescriptreact', 
      'html', 'css', 'go', 'lua'
    },
    settings = {
      rootMarkers = {".git/", 'package.json', 'Makefile', 'go.mod'},
      lintDebounce =   "2s",
      formatDebounce = "1000ms",
      languages = {
        typescript = {stylelint, prettier},
        typescriptreact = {stylelint, prettier},
        javascript = {eslint_d},
        javascriptreact = {eslint_d},
       python = { python-flake8 },
        go = {
          {
            formatCommand = "golines --max-len=120  --base-formatter=gofumpt",
            formatStdin = true,
            -- "staticcheck" ?
            lintCommand = "golangci-lint run",
            LintSeverity = 3,
          }
        },
        lua = {
          { formatCommand = "lua-format --indent-width 2 --tab-width 2 --no-use-tab --column-limit 120 --column-table-limit 100 --no-keep-simple-function-one-line --no-chop-down-table --chop-down-kv-table --no-keep-simple-control-block-one-line --no-keep-simple-function-one-line --no-break-after-functioncall-lp --no-break-after-operator",
           formatStdin = true,
          }
        },
      }
    }
  }

@mattn
Copy link
Owner

mattn commented Jul 7, 2021

Your "2s" is converted to JSON value as string for workspace/didChangeConfiguration. And it is decoded as json.Unmarshal

if err := json.Unmarshal(*req.Params, &params); err != nil {

As far as I can see, it should work correctly.

@ray-x
Copy link
Contributor Author

ray-x commented Jul 7, 2021

In fact, I am seeing an error in that file (with the config I post earlier):

2021/07/07 16:46:34 jsonrpc2 handler: notification "workspace/didChangeConfiguration" handling error: json: cannot unmarshal string into Go struct field Config.settings.lintDebounce of type time.Duration

@ray-x
Copy link
Contributor Author

ray-x commented Jul 7, 2021

Please take a look at golang/go#10275
This is a known issue that 1s in json can not be decoded to time.Duration

@mattn
Copy link
Owner

mattn commented Jul 7, 2021

Now I understood what is wrong. Thank you.

@mattn mattn merged commit de1c0d2 into mattn:master Jul 7, 2021
@mattn
Copy link
Owner

mattn commented Jul 7, 2021

Thank you

@ray-x
Copy link
Contributor Author

ray-x commented Jul 7, 2021

Cheers!

@mattn
Copy link
Owner

mattn commented Jul 8, 2021

Hmm, this break compatibility. Now efm-langserver can not load yaml configuration.

@mattn mattn mentioned this pull request Jul 8, 2021
@ray-x
Copy link
Contributor Author

ray-x commented Jul 8, 2021

What is the config.yaml setting breaks the loading?
I am using config.yaml with all settings copy from README.md. I did not see the issue

@mattn
Copy link
Owner

mattn commented Jul 8, 2021

In previous version, efm-langserver does not parse below.

lint-debounce: 1s

Fixed in #147

@ray-x
Copy link
Contributor Author

ray-x commented Jul 8, 2021

In fact. I think that part is missing in the config.yaml. Could you also update the document of how to setup lint-debounce/format-debouce in default config.yaml

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.

2 participants