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

Fix #508: Add custom variable eglot-flymake-immediate-report. #957

Closed
wants to merge 1 commit into from

Conversation

JimDBh
Copy link

@JimDBh JimDBh commented May 17, 2022

Setting it to nil would make eglot respect flymake-no-changes-timeout.

@JimDBh JimDBh changed the title Add custom variable eglot-flymake-immediate-report. Fix #508: Add custom variable eglot-flymake-immediate-report. May 17, 2022
@joaotavora
Copy link
Owner

Hmmm, I think I understand what you mean. But it's not flymake-no-change-timeout that's being ignored. The LSP server is sending multiple updates for the same request by flymake (which does respect f-n-c-t). You probably want to customize eglot-send-changes-idle-time.

@joaotavora
Copy link
Owner

Hi @JimDBh, I've been thinking about this, and I think this proposal has merit. Let's continue discussion here.

@JimDBh
Copy link
Author

JimDBh commented May 18, 2022

Yep. Although I think maybe this PR is not correct now, as I suspect if we do it this way then the flymake report would not be up to date and is always one step behind?

@joaotavora
Copy link
Owner

I haven't tested. The idea seems solid, but it has to be well tested. So I hope you could test this with the two values for the flag in a small number of LSP servers. Better yet, you could add automated tests to eglot-tests.el.

Another matter: this enhancement is small enough to not need a FSF copyright assignment, but any larger contribution (like one with tests) would need it.

@JimDBh
Copy link
Author

JimDBh commented May 18, 2022

Thanks for the reply! Yeah I'll try to test this and change it if needed. Not quite familiar with eglot-tests, but will also try to do that too.
I'll also check the copyright forms too. Will update here with anything new.

@joaotavora
Copy link
Owner

I suspect if we do it this way then the flymake report would not be up to date and is always one step behind?

Yes, exactly. I've just tested it minimally and it has that glaring problem. The problem only happens when Flymake "takes the initiative", and when flymake-no-changes-timeout is less than (eglot-send-changes-idle-time + time-it-takes-for-lsp-server-to-issue-diagnostics).

This suggests that if flymake-no-changes-timeout is large enough (or nil == infinite) then your strategy works. The problem is that time-it-takes-for-lsp-server-to-issue-diagnostics is a hard unknown and depends on many factors (project complexity, LSP server's whims, etc)

So then only way I can see this working is if flymake-no-changes-timeout is nil. In other words, this patch:

diff --git a/eglot.el b/eglot.el
index e8f060c..46b9e13 100644
--- a/eglot.el
+++ b/eglot.el
@@ -1996,7 +1996,11 @@ COMMAND is a symbol naming the command."
                                              collect it)))
                           `((face . ,faces))))))
            into diags
-           finally (cond (eglot--current-flymake-report-fn
+           finally (cond ((and
+                           ;; if f-n-c-t is nil, i.e. "infinite",
+                           ;; don't add to current report.
+                           (not (null flymake-no-changes-timeout))
+                           eglot--current-flymake-report-fn)
                           (eglot--report-to-flymake diags))
                          (t
                           (setq eglot--diagnostics diags)))))

Then you can M-x flymake-start (or use some keybinding) to get the up-to-date diagnostics that Eglot has been holding on to.

joaotavora added a commit that referenced this pull request May 19, 2022
Also per #957.

Only actually and eagerly report LSP diagnotics if the user has
Flymake starting automatically on a timer (flymake-no-changes-timeout
is a number).

By contrast, if flymake-no-changes-timeout is nil, the user starts the
diagnostic collection process on-demand via 'M-x flymake-start'.

Since the control of such collection is impossible with LSP, we should
just hold on to whatever diagnostics we have (which are presumably
up-to-date) until the next invocation of 'eglot-flymake-backend'.

For now, this doesn't affect Flymake "list-only" diagnostics.  Those
are reported via the 'flymake-list-only-diagonstics' variable and
are always communicated immediately to it.

* eglot.el: (eglot-handle-notification
textDocument/publishDiagnostics): Consult flymake-no-changes-timeout.

Suggested-by: Jim Davis <jim.jd.davis@gmail.com>
@joaotavora joaotavora closed this May 19, 2022
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 18, 2022
Also per joaotavora/eglot#957.

Only actually and eagerly report LSP diagnotics if the user has
Flymake starting automatically on a timer (flymake-no-changes-timeout
is a number).

By contrast, if flymake-no-changes-timeout is nil, the user starts the
diagnostic collection process on-demand via 'M-x flymake-start'.

Since the control of such collection is impossible with LSP, we should
just hold on to whatever diagnostics we have (which are presumably
up-to-date) until the next invocation of 'eglot-flymake-backend'.

For now, this doesn't affect Flymake "list-only" diagnostics.  Those
are reported via the 'flymake-list-only-diagonstics' variable and
are always communicated immediately to it.

* eglot.el: (eglot-handle-notification
textDocument/publishDiagnostics): Consult flymake-no-changes-timeout.

Suggested-by: Jim Davis <jim.jd.davis@gmail.com>
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
Also per joaotavora/eglot#957.

Only actually and eagerly report LSP diagnotics if the user has
Flymake starting automatically on a timer (flymake-no-changes-timeout
is a number).

By contrast, if flymake-no-changes-timeout is nil, the user starts the
diagnostic collection process on-demand via 'M-x flymake-start'.

Since the control of such collection is impossible with LSP, we should
just hold on to whatever diagnostics we have (which are presumably
up-to-date) until the next invocation of 'eglot-flymake-backend'.

For now, this doesn't affect Flymake "list-only" diagnostics.  Those
are reported via the 'flymake-list-only-diagonstics' variable and
are always communicated immediately to it.

* eglot.el: (eglot-handle-notification
textDocument/publishDiagnostics): Consult flymake-no-changes-timeout.

Suggested-by: Jim Davis <jim.jd.davis@gmail.com>
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
Also per #957.

Only actually and eagerly report LSP diagnotics if the user has
Flymake starting automatically on a timer (flymake-no-changes-timeout
is a number).

By contrast, if flymake-no-changes-timeout is nil, the user starts the
diagnostic collection process on-demand via 'M-x flymake-start'.

Since the control of such collection is impossible with LSP, we should
just hold on to whatever diagnostics we have (which are presumably
up-to-date) until the next invocation of 'eglot-flymake-backend'.

For now, this doesn't affect Flymake "list-only" diagnostics.  Those
are reported via the 'flymake-list-only-diagonstics' variable and
are always communicated immediately to it.

* eglot.el: (eglot-handle-notification
textDocument/publishDiagnostics): Consult flymake-no-changes-timeout.

Suggested-by: Jim Davis <jim.jd.davis@gmail.com>

#508: joaotavora/eglot#508
#957: joaotavora/eglot#957
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 12, 2022
Also per joaotavora/eglot#957.

Only actually and eagerly report LSP diagnotics if the user has
Flymake starting automatically on a timer (flymake-no-changes-timeout
is a number).

By contrast, if flymake-no-changes-timeout is nil, the user starts the
diagnostic collection process on-demand via 'M-x flymake-start'.

Since the control of such collection is impossible with LSP, we should
just hold on to whatever diagnostics we have (which are presumably
up-to-date) until the next invocation of 'eglot-flymake-backend'.

For now, this doesn't affect Flymake "list-only" diagnostics.  Those
are reported via the 'flymake-list-only-diagonstics' variable and
are always communicated immediately to it.

* eglot.el: (eglot-handle-notification
textDocument/publishDiagnostics): Consult flymake-no-changes-timeout.

Suggested-by: Jim Davis <jim.jd.davis@gmail.com>
GitHub-reference: fix joaotavora/eglot#508
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 20, 2022
Also per joaotavora/eglot#957.

Only actually and eagerly report LSP diagnotics if the user has
Flymake starting automatically on a timer (flymake-no-changes-timeout
is a number).

By contrast, if flymake-no-changes-timeout is nil, the user starts the
diagnostic collection process on-demand via 'M-x flymake-start'.

Since the control of such collection is impossible with LSP, we should
just hold on to whatever diagnostics we have (which are presumably
up-to-date) until the next invocation of 'eglot-flymake-backend'.

For now, this doesn't affect Flymake "list-only" diagnostics.  Those
are reported via the 'flymake-list-only-diagonstics' variable and
are always communicated immediately to it.

* eglot.el: (eglot-handle-notification
textDocument/publishDiagnostics): Consult flymake-no-changes-timeout.

Suggested-by: Jim Davis <jim.jd.davis@gmail.com>
GitHub-reference: fix joaotavora/eglot#508
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.

None yet

2 participants