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 handling of ranges at file end. #3344

Merged

Conversation

dfn-certling
Copy link
Contributor

Something along these lines solves the range ending at the file end in my case

autoload/ale/code_action.vim Outdated Show resolved Hide resolved
@AlxHnr
Copy link

AlxHnr commented Sep 14, 2020

I've run into the same issue and can confirm it. Using your fix however leads to ALE breaking my code after renaming a variable with clangd. Here is what happens if I try to to rename setValue to setValue123:

Before:

void Bar::setValue(int value) {
  this->value = value;
}

After:

void Bar::setValue123(int value) { s->value = value; 

The new code is missing } and this gets replaced with s.

Edit: See #3322

@w0rp
Copy link
Member

w0rp commented Sep 17, 2020

@AlxHnr Can you propose another patch on top of this, and attach the output of ch_logfile from Vim so we can add a test case? You have to use Vim for testing it to get a log file, as NeoVim doesn't offer message logging like Vim does.

@w0rp w0rp added this to In Review in On the Radar via automation Sep 17, 2020
@AlxHnr
Copy link

AlxHnr commented Sep 17, 2020

@AlxHnr Can you propose another patch on top of this, and attach the output of ch_logfile from Vim

No.

I assume this PR will already fix this issue. The file scrambling problem is a different one, described in #3322. I've posted a more detailed, reproducible example there.

@w0rp
Copy link
Member

w0rp commented Sep 18, 2020

Okay, I asked for the information I'll need in that issue.

This patch can be merged once there's a test that covers this. Have a look at the test cases, or I'll do it myself at some point. (Could be weeks away from now. It's just whenever I have some spare time.)

@dfn-certling
Copy link
Contributor Author

dfn-certling commented Sep 18, 2020

I'm puzzled. Looking through the tests

Execute(End of file can be modified):

seems to directly test this. And indeed, in my setup this test already fails without my changes. To be more precise, I didn't run the test suite but did what the test essentially does:

  1. Create a file with the content described in the test
  2. Call ale#code_action#HandleCodeAction with the parameters from the test

So either this test already fails or there is something else at work.

@stale
Copy link

stale bot commented Oct 16, 2020

This pull request has been automatically marked as stale because it has not been updated recently. Make sure to write tests and document your changes. See :help ale-dev for information on writing tests.
If your pull request is good to merge, bother w0rp or another maintainer again, and get them to merge it.

@stale stale bot added the stale PRs a bot will close automatically label Oct 16, 2020
@stale stale bot closed this Oct 23, 2020
On the Radar automation moved this from In Review to Done Oct 23, 2020
@w0rp
Copy link
Member

w0rp commented Nov 21, 2020

I think this has been fixed in a different way according to recent changes. Let me know if you still have any issues.

@w0rp w0rp reopened this Nov 21, 2020
On the Radar automation moved this from Done to In Review Nov 21, 2020
@stale stale bot removed the stale PRs a bot will close automatically label Nov 21, 2020
@w0rp
Copy link
Member

w0rp commented Nov 21, 2020

Apparently this is still needed, so I'll just merge it.

Cheers! 🍻

@w0rp w0rp merged commit c10e807 into dense-analysis:master Nov 21, 2020
On the Radar automation moved this from In Review to Done Nov 21, 2020
@dfn-certling dfn-certling deleted the 2801-rename-index-out-of-range branch January 22, 2021 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants