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/foldingRange #632

Merged
merged 35 commits into from Apr 10, 2019

Conversation

2 participants
@marek1840
Copy link
Collaborator

commented Apr 4, 2019

Previously, folding ranges capability was turned off and we had to rely on build in capabilities of editors.

Now we start providing out own folding ranges to the editor

@olafurpg
Copy link
Member

left a comment

Looking good! I left a couple code style comments, excited to try this out :)

@olafurpg
Copy link
Member

left a comment

Getting much better! A few more coding style comments and testing suggestions

@olafurpg olafurpg changed the title Implement folding ranges capability Implement textDocument/foldingRange Apr 4, 2019

@olafurpg
Copy link
Member

left a comment

I just tried this out locally and it's looking great! Can't wait to use this, it's a feature I've been missing from IntelliJ :)

A few notes:

I expected all println to be folded in the example below because hiding a single println(1) is not very helpful
2019-04-05 08 40 37

It would be nice to fold large Term.Function bodies like here
Screenshot 2019-04-05 at 08 46 13

Show resolved Hide resolved metals/src/main/scala/scala/meta/internal/metals/FoldingRangeProvider.scala Outdated
Show resolved Hide resolved metals/src/main/scala/scala/meta/internal/metals/FoldingRangeProvider.scala Outdated
Show resolved Hide resolved tests/unit/src/main/scala/tests/InputProperties.scala
Show resolved Hide resolved tests/unit/src/test/resources/foldingRange/expect/Blocks.scala Outdated
import scala.meta.internal.metals.FoldingRangeProvider
import scala.meta.io.AbsolutePath

object FoldingRangeSuite extends DirectoryExpectSuite("foldingRange/expect") {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Apr 5, 2019

Member

Nice job, this test suite looks great 👏

Show resolved Hide resolved metals/src/main/scala/scala/meta/internal/metals/FoldingRangeProvider.scala Outdated
@olafurpg

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

I just tried the import folding and it's amazing 😍

2019-04-06 11 45 19

@olafurpg
Copy link
Member

left a comment

Should we fold large nested multiline statements?

For example, here below we could emit three folding ranges

  checkEdit(
    "stale",
    """package stale
      |sealed abstract class Weekday
      |case object Workday extends Weekday
      |case object Weekend extends Weekday
      |object App {
      |  null.asInstanceOf[Weekday] matc@@
      |}
      |""".stripMargin,
    """|package stale
       |sealed abstract class Weekday
       |case object Workday extends Weekday
       |case object Weekend extends Weekday
       |object App {
       |  import stale.Workday
       |  import stale.Weekend
       |  null.asInstanceOf[Weekday] match {
       |\tcase Workday => $0
       |\tcase Weekend =>
       |}
       |}
       |""".stripMargin,
    filter = _.contains("exhaustive")
  )
  1. the entire check
  2. first multiline string
  3. second multiline string

If the nested range is >50% of the parent range then we can maybe skip it, for example in

  checkEdit(
    "stale",
    """package stale
      |sealed abstract class Weekday
      |case object Workday extends Weekday
      |case object Weekend extends Weekday
      |object App {
      |  null.asInstanceOf[Weekday] matc@@
      |}
      |""".stripMargin,
  )

we don't need a range for the multiline string literal

@marek1840 marek1840 force-pushed the marek1840:implement-folding-ranges branch from 708deb3 to fe3cb4e Apr 10, 2019

@olafurpg
Copy link
Member

left a comment

Two tiny comments, otherwise this PR looks ready to go!

@@ -498,4 +501,26 @@ object MetalsEnrichments
}
}

implicit class XtensionTreeTokenStream(tree: Tree) {

This comment has been minimized.

Copy link
@olafurpg
@@ -291,5 +292,12 @@ trait MtagsEnrichments {
new l.Position(pos.endLine, pos.endColumn)
)
}

def toLSPFoldingRange: FoldingRange = {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Apr 10, 2019

Member

Let's inline this since it's only used in one place

@@ -0,0 +1,35 @@
class A >>region>>{
val tryCatch =
try>>region>> ???<<region<<

This comment has been minimized.

Copy link
@olafurpg

olafurpg Apr 10, 2019

Member

Should this single-line range here be folded?

import scala.concurrent.Future

object FoldingRangeSlowSuite extends BaseSlowSuite("foldingRange") {
testAsync("parse-error") {

This comment has been minimized.

Copy link
@olafurpg
@olafurpg
Copy link
Member

left a comment

LGTM 👍 Great work @marek1840!

@olafurpg

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Ignoring flaky test failure on Appveyor tests.CompletionSlowSuite.rambo

@olafurpg olafurpg merged commit ed7ee28 into scalameta:master Apr 10, 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 #20190410.6 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 :)

@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.