Send point or region as code action range

[muffinmad]

Trying to implement refactor.inline code action provided by jedi.

Looks like eglot never send the current point on codeAction request.

This PR aimed to send region boundaries when region is active and just the point otherwise:

• Each diagnostic contains its own range, there are no need to calculate codeAction range based on diagnostics under point.
• Exact point is certainly giving more context information to server than (point-min) (point-max).

[nemethf]

There is a typo in the "subject": acrion -> action

I like this PR. (Can you add a code-action test using one of the servers run by travis?)

[muffinmad]

There is a typo in the "subject": acrion -> action

Fixed, thanks!

I like this PR. (Can you add a code-action test using one of the servers run by travis?)

Do we really need the server to test this? Isn't it enough to test that the valid request is generated?

[nemethf]

(Can you add a code-action test using one of the servers run by travis?)

Do we really need the server to test this? Isn't it enough to test that the valid request is generated?

Probably yes. Eglot has no tests in this regard, so a test of any kind is an improvement.

[joaotavora]

Andrii Kolomoiets <notifications@github.com> writes:

> I like this PR. (Can you add a code-action test using one of the > servers run by travis?)

I of course also like a pull request that deletes code and keeps the same correct behaviour, but I'm not 100% sure those boundaries calculations are useless. Maybe they weren't at the time and are now. The arguments seem sound though.

Do we really need the server to test this? Isn't it enough to test that the valid request is generated?

Perhaps it's not needed to test your particular change, but I would appreciate a test of code actions where, say, a buffer had, say, 4 diagnostics and you request code actions for the middle 2. Though I wouldn't spend an immense amount of effort there either. Does `pyls` do code actions? João

[nemethf]

There is a typo in the "subject": acrion -> action

Fixed, thanks!

I mean in the subject of the commit message. Have you fixed that as well?

[muffinmad]

I mean in the subject of the commit message. Have you fixed that as well?

Yes :)

[joaotavora]

I've thought a bit more about this. How do I tell the server to give me all code actions for the whole buffer? That used to be the default. Do I just mark the whole buffer now?

Not saying it's bad, mind you.

[muffinmad]

I've thought a bit more about this. How do I tell the server to give me all code actions for the whole buffer? That used to be the default. Do I just mark the whole buffer now?

Yes. IMO this is predictable behavior in contrast of sending whole buffer region when there are no selection.

Perhaps it's not needed to test your particular change, but I would appreciate a test of code actions where, say, a buffer had, say, 4 diagnostics and you request code actions for the middle 2.

I'll see what I can do about it.

Does pyls do code actions?

No. Action support is declared in the server capabilities but there are no actions provided.

"Inline variable" code action provided by recent anakinls :)
But that action is available only if point is under variable that must be inlined.

[joaotavora]

Yes. IMO this is predictable behavior in contrast of sending whole buffer region when there are no selection.

OK. I'm sold. I like that logic.

I'll see what I can do about it.

Do you think anakinls is easy (and quick) to install on Travis? And is free software, by the way? If so, then it'd be nice to move all the tests to anakinls :-)

[muffinmad]

Perhaps it's not needed to test your particular change, but I would appreciate a test of code actions where, say, a buffer had, say, 4 diagnostics and you request code actions for the middle 2.

Done. Not exactly with those diagnostic counts but see basic-actions-point and basic-actions-selection tests.

Do you think anakinls is easy (and quick) to install on Travis?

I think so. Just pip install.

And is free software, by the way?

Yes.

If so, then it'd be nice to move all the tests to anakinls :-)

anakinls will not pass some tests: it doesn't do code formatting (yet :-)), completions works in slightly different way and so on.

IMO more important question is if we need the server in the tests at all.

It is enough to test that eglot is making correct requests and correctly handle responses. I mean there are no need to install pyls and autopep8 to test how TextDocumentEdit is applied to the buffer.

Because otherwise we testing servers :-) E.g. at the moment the travis config or the rename-a-symbol test must be fixed because the latest pyls give us No support for renaming in Python 2/3.5 with Jedi. Consider using the rope_rename plugin instead error. pyls update breaks eglot tests. Not good.

[joaotavora]

Because otherwise we testing servers :-)

We need to do that. We need real-life fixtures, not idealized ones.

[joaotavora]

pyls update breaks eglot tests. Not good.

Indeed I agree. Can you help us "pin" the pyls version in Travis?

[muffinmad]

Because otherwise we testing servers :-)

We need to do that. We need real-life fixtures, not idealized ones.

Got it. In this case it make sense to test how action actually applied to the buffer. I need your advice here: how can the test pick one of the actions returned by the server?

[muffinmad]

pyls update breaks eglot tests. Not good.

Indeed I agree. Can you help us "pin" the pyls version in Travis?

Fixed on master branch.

[muffinmad]

Friendly ping

[joaotavora]

Andrii Kolomoiets <notifications@github.com> writes:

Friendly ping

Hi Andrii, I've had a better look at this: - I'm wasn't sure this PR wouldn't be breaking the [mouse-1] left-click action on diagnotics. The one established by: (define-key map [mouse-1] (eglot--mouse-call 'eglot-code-actions)) But I checked and it seems not to break, which is good news. The reason it works is that flymake-diagnostics will defer to overlays-in, which _will_ return some overlays. However, the docstring of overlays-in isn't explicit about this. Something to fix in that docstring, I suppose, so not our problem. But regardless, I think it's better to default BEG to current point and END to nil. - The docstring of eglot-code-actions has to be tweaked to say so. I'll perform both these changes, squash the two commits together and push, if I find no problems. João

[joaotavora]

Hi again, Andrii João Távora <joaotavora@gmail.com> writes:

Andrii Kolomoiets ***@***.***> writes: > Friendly ping But I checked and it seems not to break, which is good news.

Actually, it _does_ appear to break, so I fixed it. I think it's slightly better to have this protocol: (defun eglot-code-actions (beg &optional end) "Offer to execute code actions between BEG and END. Interactively, if a region is active, BEG and END are its bounds, else BEG is point and END is nil, which results in a request for code actions at point" (interactive (if (region-active-p) `(,(region-beginning) ,(region-end)) `(,(point) nil))) which, of course, has the very same effect that you wanted. Now, I pushed this under your name with a "Co-authored-by:"" tag (let me know if you don't like this). I didn't push the tests. Not because I don't like them, but because I think these have some problems. First, the code needs to be indented. Do this by making sure all the macros are loaded and then indenting the region with the code. Now some specific comments: + (flymake-start) It would be nice to have a comment here saying that you do this to make the diagnostics waited on appear as actual Flymake diagnostics. + (goto-char 94) "94" seems waaay to brittle. Isn't there a better way? + (ignore-errors (call-interactively (eglot-code-actions))) We certainly don't want ignore-errors, and it's better just to call: (eglot-code-actions (point)) , becasue, in lisp programs such as tests, it's safer to rely on eglot.el's Lisp interface, not the interactive specs (or at least not like this). It's true there is an exception with xref-find-definitions, but it's better not to repeat it. + (eglot--wait-for + (c-reqs) + (&key _id method params &allow-other-keys) + (and (string-equal method "textDocument/codeAction") + (equal (plist-get params :range) + '(:start + (:line 5 :character 10) + :end + (:line 5 :character 10))) + (let ((diags (plist-get + (plist-get params :context) + :diagnostics))) + (and (= (length diags) 1) + (equal (plist-get (aref diags 0) :range) + '(:start + (:line 5 :character 0) + :end + (:line 5 :character 19))))))))))) It's OK to wait for the client request to go to the server, but the specific wait for a request that meets exactly those characteristics seems off to me. So basically I would only wait for: (string-equal method "textDocument/codeAction") Also, I wouldn't check the nitty gritty details of this request which, again, are brittle. It's better to check more generic stuff, such as (= (length diags) 1) But I would do this with an `is` assertion after the wait. If you prefer, you can even continue the test in the sense that you eventually invoke the code-action request and then verify its results in the buffer. + ;; selection as range and no diagnostic + (set-mark 0) + (goto-char 8) Same comment as above + (activate-mark) No need for this or the set-mark, I think. See below. + (eglot--sniffing + (:client-requests c-reqs) + (ignore-errors (call-interactively #'eglot-code-actions)) It's better to (eglot-code-actions (region-beginning) (region-end)) João

[muffinmad]

Actually, it does appear to break, so I fixed it.

Thanks!

which, of course, has the very same effect that you wanted.

Can confirm, works as expected.

First, the code needs to be indented. Do this by making sure all the
macros are loaded and then indenting the region with the code.

My bad. Didn't know about indent at the moment.

(flymake-start)

It would be nice to have a comment here saying that you do this to make
the diagnostics waited on appear as actual Flymake diagnostics.

Got it.

(goto-char 94)

"94" seems waaay to brittle. Isn't there a better way?

Agree. I think search-forward will fit better here.

 (ignore-errors (call-interactively (eglot-code-actions)))

We certainly don't want ignore-errors, and it's better just to call:

Not with pyls which returns no actions. Test will fail with the No code actions here error.

(eglot-code-actions (point))

, becasue, in lisp programs such as tests, it's safer to rely on
eglot.el's Lisp interface, not the interactive specs (or at least not
like this). It's true there is an exception with xref-find-definitions,
but it's better not to repeat it.

But my goal was to test that interactive code.

It's OK to wait for the client request to go to the server, but the
specific wait for a request that meets exactly those characteristics
seems off to me. So basically I would only wait for:

(string-equal method "textDocument/codeAction")

Also, I wouldn't check the nitty gritty details of this request which,
again, are brittle. It's better to check more generic stuff, such as

(= (length diags) 1)

But I would do this with an is assertion after the wait.

I'm verifying here that the diagnostic from the current line is sent and not from the first line. Thats the reason to check all the details.

If you prefer, you can even continue the test in the sense that you
eventually invoke the code-action request and then verify its results in
the buffer.

I need your advice here: how can the test pick one of the actions returned by the server?

(eglot--sniffing
 (:client-requests c-reqs)
 (ignore-errors (call-interactively #'eglot-code-actions))

It's better to

(eglot-code-actions (region-beginning) (region-end))

Again, I wanted to test the interactive part; verify that request is composed correctly when no arguments for eglot-code-actions is specified. In case there are no need in such test, we surely can write some other test to just check buffer text after applying code action.

One more thing: should I continue work in this PR or better move the test to the new one?

Thanks again!