Use eglot-lsp-abiding-column

[theothornhill]

I noticed you use the same calculation in two spots - maybe it would be better for refactorability later if it only resided in one place?

[joaotavora]

Indeed this makes a lot of sense, I don't know what I was thinking. But also please double-, nay, triple-, nay quadruple-check that the code is exactly the same. And wouldn't it be great to add some tests, too? :-)

[theothornhill]

I will look into tests! I've checked a couple of times already, but yeah, will do again :)

[joaotavora]

The thing is you will need a server, probably clangd, to test this specific thing with some non-ascii text. No biggie, just bear in mind that you need a UTF-16, perfectly LSP compliant server. clangd is probably the best choice. See the existing tests for how to make a test conditional on the presence of a particular server.

Also see #361. This will help us make these functions the default.

[theothornhill]

The thing is you will need a server, probably clangd, to test this specific thing with some non-ascii text. No biggie, just bear in mind that you need a UTF-16, perfectly LSP compliant server. clangd is probably the best choice. See the existing tests for how to make a test conditional on the presence of a particular server.

Thanks for the tip!

Also see #361. This will help us make these functions the default.

I'll check it out

[theothornhill]

Hello!
I've been looking into the tests, and the latest two commits seems to run fine on my computer - they don't in travis. I wondered if maybe you see something of note here.

First:

• The first commit with a test is failing because I didn't set eglot-current-column-function in test body. I didn't fix this since I'd rather get the column number itself, instead of getting the whole json object. I left the commit there mostly so that I get a diff in next commit

• The second commit should run fine. However, it returns end-of-buffer from eglot-move-to-lsp-abiding-column. This surprises me a little, since I've run the steps one by one in emacs -Q manually, and the tests run both with and without config locally.

I started wondering if the second case here is a bug in eglot, since eglot-move-to-lsp-abiding-column only runs a cl-loop without error handling. Maybe that shouldn't matter though.

[joaotavora]

I've been looking into the tests, and the latest two commits seems to run fine on my computer - they don't in travis. I wondered if maybe you see something of note here.

It might be the clangd version that travis offers. Maybe you can conditionalize the test further to check on the clagnd version and/or explore Travis's documentation to get an updated clangd installed. I have the clangd-9 ubuntu package installed, not sure if that is available in travis.

[joaotavora]

I might have read your message too diagonally. Let me ask you something:

1. By "running fine on your machine", do you mean the tests finish and pass on your machine?
2. Do those two observations result from your investigations into Travis' output?

[joaotavora]

Another question, are you running the check via make check? Did you know you can run just tests matching a certain regexp using make check SELECTOR='"rename"' or run them using a particular Emacs version using make check SELECTOR='"rename"' EMACS=path/to/my/preferred/emacs

[theothornhill]

It might be the clangd version that travis offers.

I’ll check, however, the should clause is successful, so clangd returns column 71, as it should.

By "running fine on your machine", do you mean the tests finish and pass on your machine?

Yes, the test passes locally, though I run it interactively through Emacs. I’ll check if make returns different stuff. I read Travis output, and some of my earlier tests returned the correct response from server.

As a side note, if I hard code in column 74 to the eglot-move then I get the end of buffer error. I guess forward-char is the culprit

[theothornhill]

@joaotavora
Ok, I confirmed that travis ships with clangd-7, and the test fails with clangd-7 (tested locally). It runs on 8+, but I cannot get travis to use it, even though it should install during build on latest commit.

Right now It just skips the test.

Any ideas? Should I try to make test pass on clangd-7? I don't know whether there are any breaking changes between them

[joaotavora]

Should I try to make test pass on clangd-7? I

No. Make travis skip the test on a version check, if possible. Don't make heroic changes to .travis.yml just to get this. (small changes are OK though).

Eventually travis will be upgraded to use clangd-8 or sth like that.

Now you just have to clean up the commits and submit again for review. Maybe make two separate commits, one for the code cleanup another that adds tests.

[joaotavora]

And thanks for all this great work!

[theothornhill]

No. Make travis skip the test on a version check, if possible. Don't make heroic changes to .travis.yml just to get this. (small changes are OK though).

Done

I'm not sure if this answers anything regarding #361, but I can confirm on my end that these functions at least work out of the box for clangd-8.

And thanks for all this great work!

My pleasure - I'm just having fun :)

[nemethf]

@theothornhill, I know this PR had many iterations, but can you, please, change the test to use looking-at instead of thing-at-point. That's what the other tests use and looking-at is a bit more precise as well. thing-at-point returns "p" when the point is on "p", but it also returns "p" and when the point is after "p". Thank you.

[theothornhill]

@nemethf

I know this PR had many iterations, but can you, please, change the test to use looking-at instead of thing-at-point

No worries, done.

However, reading docs for looking-at, it seems like what we really want here is looking-at-p

This function does not move point, but it does update the match data. See Match Data. If you need to test for a match without modifying the match data, use looking-at-p

It may not be an important thing, but just found it interesting anyhow. I changed it to looking-at for consistency

[theothornhill]

@nemethf
Also I see a check is failing, but It is not related to my test. It is basic-diagnostics, which is related to pyls Could pyls not be lsp-abiding, since that commit now is in master?

Seems like it is mentioned here: #125 (comment)

[nemethf]

Damn. I always make this mistake to not wait for the test results. I'll investigate.

[nemethf]

I know this PR had many iterations, but can you, please, change the test to use looking-at instead of thing-at-point

No worries, done.

Thank you. I'm OK with this PR now. However, it seems you need your copyright paperwork finished before it's possible the merge the PR.

However, reading docs for looking-at, it seems like what we really want here is looking-at-p

That's a question for a separate issue, I guess. But since the file uses looking-back which doesn't have a -p variant, I'm not so sure about the change.

(In the meantime, I reverted the column-function change in the master branch.)

[theothornhill]

Thank you. I'm OK with this PR now. However, it seems you need your copyright paperwork finished before it's possible the merge the PR.

On it :)

[theothornhill]

Hello! I saw that 1.6 was released, and the lsp-abiding-column thing was part of that.

Is this test still wanted?

I also see the algorithm is changed a bit, as per the #361 discussion.

I haven't been able to follow up on this for a while (sorry for that) :(

[joaotavora]

Is this test still wanted?

Tests are always wanted. Let me check if it passes with clangd-9 and/or travis.

Thanks.

[joaotavora]

I rebased your stuff and am about to push it. The test seems a bit brittle (it uses a bleeding edge setq-local stuff, and all that caaddr and caddrdr is hard to follow), but it's not bad, I think. I suspect it's always going to pass in Travis, since it doesn't have any clangd. See the travis-fiddling branch. I put Co-authored-by: <me> in your commits, since I tweaked them.

[joaotavora]

it's always going to pass in Travis, since it doesn't have any clangd.

I was wrong. It does have clangd and recent enough. Tests pass so I'm happy, going to push. thanks!