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

tools/gopls: add cmd support for prepare_rename #188

Closed
wants to merge 1 commit into from

Conversation

rhnvrm
Copy link
Contributor

@rhnvrm rhnvrm commented Nov 17, 2019

This change adds command line support for prepare_rename.

Updates golang/go#32875

@gopherbot
Copy link
Contributor

This PR (HEAD: 2c9d27f) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/207579 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Rohan Verma:

Patch Set 1:

A few of the tests are failing, I am not able to figure out what could be the cause. Could someone please guide me about how to check what could be the cause.

The following link contains the result of go test ./internal/lsp/cmd
https://pastebin.com/nhS4Cku7


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Francesco Renzi:

Patch Set 1:

Patch Set 1:

A few of the tests are failing, I am not able to figure out what could be the cause. Could someone please guide me about how to check what could be the cause.

The following link contains the result of go test ./internal/lsp/cmd
https://pastebin.com/nhS4Cku7

It might be that you have an error in your cmd. In your tests you can see such errors in the output of tool.Run, see an example here https://github.com/golang/tools/blob/master/internal/lsp/cmd/test/folding_range.go#L19-L22


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Francesco Renzi:

Patch Set 1:

(4 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 77e6411) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/207579 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Rohan Verma:

Patch Set 2:

Patch Set 1:

(4 comments)

Hey

I updated the tests and fixed the issues you mentioned.


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Francesco Renzi:

Patch Set 2:

(4 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Francesco Renzi:

Patch Set 2: Code-Review+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rebecca Stambler:

Patch Set 2:

(3 comments)

Thank you for your contribution! Just a few small comments.


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 56bfde9) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/207579 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Rohan Verma:

Patch Set 3:

Patch Set 2:

(3 comments)

Thank you for your contribution! Just a few small comments.

Hey

Thanks for the review. I have updated the commit.


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rebecca Stambler:

Patch Set 3:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 9a8bd3b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/207579 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Rebecca Stambler:

Patch Set 4:

Rohan, do you intend to keep working on this CL or would you prefer if a member of the Go team took it over?


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: d4de61b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/207579 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Rohan Verma:

Patch Set 5:

(1 comment)

Patch Set 4:

Rohan, do you intend to keep working on this CL or would you prefer if a member of the Go team took it over?

Hey

I won't mind, but I can keep working on it.

So I have rebased and updated the patch, although since the patch got stale, the underlying generated code for the protocol has also changed and is now an interface{} instead of a Range.

5091d64#diff-858a28237fff8f7148599ba89e7ed53dR825


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rebecca Stambler:

Patch Set 5:

(1 comment)

Patch Set 5:

(1 comment)

Patch Set 4:

Rohan, do you intend to keep working on this CL or would you prefer if a member of the Go team took it over?

Hey

I won't mind, but I can keep working on it.

So I have rebased and updated the patch, although since the patch got stale, the underlying generated code for the protocol has also changed and is now an interface{} instead of a Range.

5091d64#diff-858a28237fff8f7148599ba89e7ed53dR825

Thanks for following up! Just one more comment, but otherwise this looks good to me.


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 571a89b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/207579 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Rohan Verma:

Patch Set 6:

I have updated the patch, although I had to make a change to the protocol file (with the generated code). I doubt that it is the correct thing to do.

Most of the tests passed except the following. I am not sure as of now, how to solve this one.

  • TestCommandLine/GOPATH/PrepareRenames/good1_5_35
  • TestCommandLine/Modules/PrepareRenames/good1_5_35

Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rebecca Stambler:

Patch Set 6: Run-TryBot+1 Code-Review+2

(2 comments)

Sorry for my delayed response! I added comments with suggestions on how to fix the test failures, but there's still one test failing (TestCommandLine/GOPATH/PrepareRenames/good1_5_35). If you'd like to remove that test from golang.org/x/tools/internal/lsp/testdata/good/good1.go, I think that would be fine, since the ranges in the cmd tests aren't very precise, and we may need to add some hack to handle them.


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 6:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=6a777a33


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 6:

Build is still in progress...
This change failed on linux-amd64 (Go 1.13.x):
See https://storage.googleapis.com/go-build-log/3e83d307/linux-amd64_8093bb66.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 6: TryBot-Result-1

10 of 10 TryBots failed:
Failed on linux-amd64 (Go 1.13.x): https://storage.googleapis.com/go-build-log/3e83d307/linux-amd64_8093bb66.log
Failed on linux-386: https://storage.googleapis.com/go-build-log/8390c478/linux-386_52b788b8.log
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/8390c478/freebsd-amd64-12_0_61992f03.log
Failed on linux-amd64 (Go 1.12.x): https://storage.googleapis.com/go-build-log/4af1337d/linux-amd64_05249d75.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/8390c478/windows-amd64-2016_29ebcb1a.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/8390c478/linux-amd64_f68f5d53.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/8390c478/windows-386-2008_61cb7aff.log
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/8390c478/openbsd-amd64-64_1c51fa3d.log
Failed on linux-amd64-race: https://storage.googleapis.com/go-build-log/8390c478/linux-amd64-race_9f7fc247.log
Failed on android-amd64-emu: https://storage.googleapis.com/go-build-log/8390c478/android-amd64-emu_5fd55fc1.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 176b6cd) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/207579 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Rohan Verma:

Patch Set 7:

I have updated the CL.

Just wanted to ask if the tsserver.go change is okay? I was not sure about that as it mentions that it was a generated file on top.


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rebecca Stambler:

Patch Set 7: Run-TryBot+1

Patch Set 7:

I have updated the CL.

Just wanted to ask if the tsserver.go change is okay? I was not sure about that as it mentions that it was a generated file on top.

Yeah, it's not a problem. We will just make sure to fix the generator when we need to regenerate the code.

I'm sorry it looks like some merge conflicts came in over the weekend. Do you mind rebasing your CL and then I can submit it?


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 7:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=530e89f6


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 7: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 118e846) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/207579 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Rebecca Stambler:

Patch Set 8: Run-TryBot+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 8:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=3ce70cd7


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rohan Verma:

Patch Set 8:

(1 comment)

Patch Set 7: Run-TryBot+1

Patch Set 7:

I have updated the CL.

Just wanted to ask if the tsserver.go change is okay? I was not sure about that as it mentions that it was a generated file on top.

Yeah, it's not a problem. We will just make sure to fix the generator when we need to regenerate the code.

Okay.

I'm sorry it looks like some merge conflicts came in over the weekend. Do you mind rebasing your CL and then I can submit it?

Alright, I've rebased and update the CL.


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 8:

Build is still in progress...
This change failed on linux-amd64 (Go 1.12.x):
See https://storage.googleapis.com/go-build-log/249c282c/linux-amd64_3f65e3e2.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 8: TryBot-Result-1

1 of 10 TryBots failed:
Failed on linux-amd64 (Go 1.12.x): https://storage.googleapis.com/go-build-log/249c282c/linux-amd64_3f65e3e2.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.


Please don’t reply on this GitHub thread. Visit golang.org/cl/207579.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request Feb 4, 2020
This change adds command line support for prepare_rename.

Updates golang/go#32875

Change-Id: I7f155b9c8329c0faa26a320abab162730a7916ad
GitHub-Last-Rev: 118e846
GitHub-Pull-Request: #188
Reviewed-on: https://go-review.googlesource.com/c/tools/+/207579
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/207579 has been merged.

@gopherbot gopherbot closed this Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants