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

Implement textDocument/documentHighlight #621

Merged
merged 1 commit into from Apr 5, 2019

Conversation

4 participants
@tgodzik
Copy link
Collaborator

commented Apr 2, 2019

Previously, we didn't support documentHighlighting from metals lsp server and symbols were only highlighted based on their name.
Now we highlight only the same symbols.

@tgodzik tgodzik force-pushed the tgodzik:add-document-highlighting branch from b08bb08 to 62a5972 Apr 2, 2019

@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 2, 2019

I haven't implemented Write or Read kinds, just used the Text one - so we might need to improve that (though to be fair not sure how to do that properly). And I found that when we have a var in class/object this will also not work when reassigning value since we invoke a method _=

@olafurpg
Copy link
Member

left a comment

Very exciting! I just gave it a try locally and it's working well 👍

Only nitpick comments on style, there are a lot of undocumented conventions in the codebase that I did my best to explain, please let me know if anything is unclear!

@olafurpg

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

We have special handling for vars in referencesProvider, I am wondering if we can try to use ReferencesProvider to implement document highlight 🤔 That would solve a few tricky cases out of the box like parameters of case class copy/apply, etc.

@olafurpg

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

To elaborate, here is a test case that should be automatically handled by references provider

case class User(<<name>>: String)
object Main {
  val user = User(<<na@@me>> = "Susan")
  println(user.<<name>>)
  user.copy(<<name>> = "John")
}

@tgodzik tgodzik force-pushed the tgodzik:add-document-highlighting branch from 5037df7 to cd62d1a Apr 3, 2019

@olafurpg olafurpg changed the title Add documentHighlighting to metals LSP server Implement textDocument/documentHighlight Apr 4, 2019

@tgodzik tgodzik force-pushed the tgodzik:add-document-highlighting branch 4 times, most recently from 3f192f4 to 228731f Apr 4, 2019

@olafurpg
Copy link
Member

left a comment

I just tried this locally and it's looking much better than before, nice job handling the trickier cases around synthetic symbols 👏 Only blocking comments are MatchError, rest is just nitpicks on coding style

Show resolved Hide resolved ...rc/main/scala/scala/meta/internal/metals/DocumentHighlightProvider.scala
@@ -0,0 +1,113 @@
package tests

object DocumentHighlightSlowSuite extends BaseSlowSuite("documentHighlight") {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Apr 5, 2019

Member

Great job with the test suite 👏

check(
"var",
"""
|case class User(var name: String)

This comment has been minimized.

Copy link
@olafurpg

olafurpg Apr 5, 2019

Member

Impressive 😍

Show resolved Hide resolved mtags/src/main/scala/scala/meta/internal/mtags/MtagsEnrichments.scala
Show resolved Hide resolved ...rc/main/scala/scala/meta/internal/metals/DocumentHighlightProvider.scala
Descriptor.Method(paramName + setterSuffix, "()")
)
)
}

This comment has been minimized.

Copy link
@olafurpg

olafurpg Apr 5, 2019

Member

Can we get MatchError here?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Apr 5, 2019

Author Collaborator

this method is just used to generate all possible alternatives - no matches here.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Apr 5, 2019

Member

Whoops, look good!

Show resolved Hide resolved ...rc/main/scala/scala/meta/internal/metals/DocumentHighlightProvider.scala
Show resolved Hide resolved ...rc/main/scala/scala/meta/internal/metals/DocumentHighlightProvider.scala Outdated
Show resolved Hide resolved ...rc/main/scala/scala/meta/internal/metals/DocumentHighlightProvider.scala Outdated
Show resolved Hide resolved ...rc/main/scala/scala/meta/internal/metals/DocumentHighlightProvider.scala Outdated

@tgodzik tgodzik force-pushed the tgodzik:add-document-highlighting branch from 228731f to 1f650cd Apr 5, 2019

Add documentHighlighting to metals LSP server
Previously, we didn't support documentHighlighting from metals lsp server and symbols were only highlighted based on their name.
Now we highlight only the same symbols. This includes highlighting both class and it's companion object, also work for parameters and vars.

@tgodzik tgodzik force-pushed the tgodzik:add-document-highlighting branch from 1f650cd to 6f03d22 Apr 5, 2019

@olafurpg
Copy link
Member

left a comment

LGTM 👍 Fantastic work @tgodzik, congrats on your first Metals PR 🎉

Descriptor.Method(paramName + setterSuffix, "()")
)
)
}

This comment has been minimized.

Copy link
@olafurpg

olafurpg Apr 5, 2019

Member

Whoops, look good!

@olafurpg

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Ignoring flaky test tests.CompletionSlowSuite.symbol-prefixes on appveyor

@olafurpg olafurpg merged commit c24eb4d into scalameta:master Apr 5, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
scalameta.metals Build #20190405.4 succeeded
Details

olafurpg added a commit to olafurpg/metals that referenced this pull request Apr 10, 2019

Welcome Marek and Tomasz to the team!
As part of a new collaboration between the Scala Center and VirtusLab,
Marek and Tomasz will be contributing to Metals development for the
coming months. They have already contributed several impressive pull
requests:

- `textDocument/foldingRange` (scalameta#632): code folding that understands Scala syntax.
- `textDocument/documentHighlight` (scalameta#621): highlight occurrences of a symbol in
  the current file.
- `textDocument/completion` (scalameta#640): override def completions from
  without the need to type "override def ".

It is my pleaseure to welcome them to the team and I look forward to see
what more comes out of this collaboration :)

olafurpg added a commit to olafurpg/metals that referenced this pull request Apr 10, 2019

Welcome Marek and Tomasz to the team!
As part of a new collaboration between the Scala Center and VirtusLab,
Marek and Tomasz will be contributing to Metals development for the
coming months. They have already contributed several impressive pull
requests:

- `textDocument/foldingRange` (scalameta#632): code folding that understands Scala syntax.
- `textDocument/documentHighlight` (scalameta#621): highlight occurrences of a symbol in
  the current file.
- `textDocument/completion` (scalameta#640): override def completions from
  without the need to type "override def ".

It is my pleasure to welcome them to the team and I look forward to
working together with them to improve the Scala editing experience :)

olafurpg added a commit to olafurpg/metals that referenced this pull request Apr 10, 2019

Welcome Marek and Tomasz to the team!
As part of a new collaboration between the Scala Center and VirtusLab,
Marek and Tomasz will be contributing to Metals development for the
coming months. They have already contributed several impressive pull
requests:

- `textDocument/foldingRange` (scalameta#632): code folding that understands Scala syntax.
- `textDocument/documentHighlight` (scalameta#621): highlight occurrences of a symbol in
  the current file.
- `textDocument/completion` (scalameta#640): override def completions from
  without the need to type "override def ".

It is my pleasure to welcome them to the team and I look forward to
working together with them to improve the Scala editing experience :)

@tgodzik tgodzik deleted the tgodzik:add-document-highlighting branch Apr 10, 2019

@olafurpg olafurpg added this to the Metals v0.5 - Mercury milestone Apr 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.