Add "Create new symbol '$name'..." code action #1528

gabro · 2020-03-23T10:20:50Z

This PR is a proposal to add a "Create new..." code action which is a quickfix for a "missing symbol" diagnostic.

Previously we would propose to import a symbol (if available) in response to "missing symbol" errors. Now we also propose to create a new class/trait/object

Here's a demo:

As you can see in the demo, this is very useful when sketching down a domain model, which requires creating a bunch of case classes in a top-down fashion.

What do you all think?

(This is currently based on top of #1525, but I'll rebase once it's merged)

gabro · 2020-03-23T10:23:00Z

metals/src/main/scala/scala/meta/internal/metals/NewFilesProvider.scala

-  def createNewFileDialog(directoryUri: Option[URI]): Future[Unit] = {
+  def createNewFileDialog(
+      directoryUri: Option[URI],
+      name: Option[String]


I've changed this to also accept a name, so that we can skip the name-picking dialog and just ask for a file kind.

This allows the code action to fully re-use the existing provider's flow, which I think is really nice

gabro · 2020-03-23T10:24:37Z

metals/src/main/scala/scala/meta/internal/metals/codeactions/CreateNewFile.scala

+
+    val codeActions = params.getContext().getDiagnostics().asScala.collect {
+      case d @ ScalacDiagnostic.SymbolNotFound(name)
+          if d.getRange().encloses(params.getRange()) =>


this is encloses and not overlapsWith, because I don't think it makes sense to show the "Create new..." action when the user has selected a range which contains multiple SymbolNotFound diagnostics.

gabro · 2020-03-23T10:25:27Z

tests/unit/src/main/scala/tests/TestingClient.scala

+  def applyCodeAction(codeAction: CodeAction, server: TestingServer): Unit = {
+    val edit = codeAction.getEdit()
+    val command = codeAction.getCommand()
+    if (edit != null) applyWorkspaceEdit(edit)
+    if (command != null) executeServerCommand(command, server)


This is now as per the LSP specification: if edit and command are both present, first edit is applied then command is executed.

olafurpg

This is an awesome idea!

olafurpg · 2020-03-23T11:10:52Z

metals/src/main/scala/scala/meta/internal/metals/codeactions/CreateNewFile.scala

+}
+
+object CreateNewFile {
+  val title = "Create new..."


create new symbol 'NAME'?

I thought about adding something like that, but I feel like "symbol" is not very user-friendly (it's very much compiler lingo).

Anything else would require listing out the options ("Create new trait 'NAME'", "Create new class 'NAME'"), but think it would become too heavy.

I feel like "Create new..." is clear when presented in context and it avoids the problem of listing out all the options.

I agree "symbol" is not ideal but it's used in "rename symbol" and "go to symbol in file" in VS Code so I think it's OK to use it here.

You're right, it's used around, at least in VSCode. I'll make the change

Nice! I think it's a bit more self documenting what this option does with the name. Should we drop the ... suffix?

The ... is used to convey the fact that the action starts a flow and it doesn't take effect immediately.

I've seen this pattern used quite often, including in VSCode. See for instance

The commands ending with ... start a flow that requires further interaction to complete.

tgodzik

Just some minor comments, but I think it's a great idea! Thanks @gabro !

tests/unit/src/test/scala/tests/codeactions/CreateNewFileLspSuite.scala

metals/src/main/scala/scala/meta/internal/metals/NewFilesProvider.scala

gabro · 2020-03-23T11:41:18Z

metals/src/main/scala/scala/meta/internal/metals/NewFilesProvider.scala

    s"""|$kind $name {
-        |  
+        |$indent


My editor is set to strip out whitespaces on save, so I've been struggling with this.

I think it's much clearer this way, anyway.

I agree this is clearer. I also have my editor remove trailing whitespace on save, intellij has this enabled by default, so we should not rely on it in our tests

I also like this as I get a warning in vim about trailing whitespaces all the time ha.

tgodzik

LGTM!

ckipp01

👍 this is great LGTM

ckipp01 · 2020-03-23T12:30:24Z

metals/src/main/scala/scala/meta/internal/metals/NewFilesProvider.scala

    s"""|$kind $name {
-        |  
+        |$indent


I also like this as I get a warning in vim about trailing whitespaces all the time ha.

metals/src/main/scala/scala/meta/internal/metals/codeactions/CreateNewFile.scala

gabro · 2020-03-23T14:29:18Z

The failed test on Windows looks relevant, but I think it's flaky (especially because it succeeded on Linux and macOS): probably a race condition that caused the diagnostic not to disappear on time.

2020-03-23T14:14:44.3224758Z [error] ==> X tests.codeactions.CreateNewSymbolLspSuite.trait  6.271s munit.FailException: D:\a\metals\metals\tests\unit\src\test\scala\tests\codeactions\CreateNewSymbolLspSuite.scala:30
2020-03-23T14:14:44.4127307Z [error] 29:
2020-03-23T14:14:44.4133840Z [error] 30:  checkNewSymbol(
2020-03-23T14:14:44.4134203Z [error] 31:    "trait",
2020-03-23T14:14:44.4134455Z [error] diff assertion failed
2020-03-23T14:14:44.4134698Z [error] => Obtained
2020-03-23T14:14:44.4134967Z [error]     """|a/src/main/scala/a/A.scala:3:43: error: not found: type Location
2020-03-23T14:14:44.4135562Z [error]        |case class School(name: String, location: Location)
2020-03-23T14:14:44.4135868Z [error]        |                                          ^^^^^^^^
2020-03-23T14:14:44.4136127Z [error]        |""".stripMargin
2020-03-23T14:14:44.4136585Z [error] => Diff (- obtained, + expected)
2020-03-23T14:14:44.4136910Z [error] -a/src/main/scala/a/A.scala:3:43: error: not found: type Location
2020-03-23T14:14:44.4137203Z [error] -case class School(name: String, location: Location)
2020-03-23T14:14:44.4137468Z [error] -                                          ^^^^^^^^
2020-03-23T14:14:44.4148767Z [error] +
2020-03-23T14:14:44.4149080Z [error]     at munit.Assertions.fail(Assertions.scala:134)

I'll restart and see whether it fixes itself

kpbochenek · 2020-03-23T15:39:55Z

Do you think it would be possible to move cursor into final case class MyClass(<<here>>) ?

From your gif it seems like you want to be there anyway and I would like to be there too if I created a new class :) (not sure how hard it is though and if it is metals or more editor specific action?)

gabro · 2020-03-23T16:00:56Z

@kpbochenek that's absolutely what I would like, and I think it should be possible by working with the range here

metals/metals/src/main/scala/scala/meta/internal/metals/NewFilesProvider.scala

Lines 192 to 193 in 8141a99

    
             new Location(path.toURI.toString(), new Range()): Object 
        
           ).asJava

I would do it in a separate issue since it's quite independent from this PR.

gabro · 2020-03-23T16:08:37Z

Ok, now the CI is failing for an unrelated test (I've fixed the previous race condition, which was my fault)

2020-03-23T15:26:33.1840250Z [error] ==> X tests.HoverLspSuite.basic-rambo  2.375s munit.FailException: D:\a\metals\metals\tests\unit\src\test\scala\tests\HoverLspSuite.scala:35 Obtained empty output!
2020-03-23T15:26:33.1840824Z [error] 34:      )
2020-03-23T15:26:33.1841093Z [error] 35:      _ <- server.assertHover(
2020-03-23T15:26:33.1841345Z [error] 36:        "a/src/main/scala/a/Main.scala",
2020-03-23T15:26:33.1873104Z [error]     at munit.Assertions.fail(Assertions.scala:134)

Merging

gabro commented Mar 23, 2020

View reviewed changes

gabro requested review from tgodzik, olafurpg and ckipp01 March 23, 2020 10:25

Add CreateNewFile code action

065e6bd

gabro force-pushed the new-file-action branch from 866423f to 065e6bd Compare March 23, 2020 10:58

olafurpg approved these changes Mar 23, 2020

View reviewed changes

Fix indents

4d4d15a

tgodzik requested changes Mar 23, 2020

View reviewed changes

tests/unit/src/test/scala/tests/codeactions/CreateNewFileLspSuite.scala Outdated Show resolved Hide resolved

metals/src/main/scala/scala/meta/internal/metals/NewFilesProvider.scala Outdated Show resolved Hide resolved

gabro commented Mar 23, 2020

View reviewed changes

gabro added 2 commits March 23, 2020 12:49

Add one more test

4fdc204

Handle blank file name

8141a99

gabro requested a review from tgodzik March 23, 2020 12:05

s/Create new.../Create new symbol '$name'...

168e9ca

gabro force-pushed the new-file-action branch from 8a690f8 to 168e9ca Compare March 23, 2020 12:17

gabro changed the title ~~"Create new..." code action~~ Add "Create new symbol '$name'..." code action Mar 23, 2020

tgodzik approved these changes Mar 23, 2020

View reviewed changes

ckipp01 approved these changes Mar 23, 2020

View reviewed changes

s/CreateNewFile/CreateNewSymbol/g

babc399

Fix race condition

29e8820

gabro mentioned this pull request Mar 23, 2020

Move cursor to a more convenient location when creating a new file #1529

Closed

gabro merged commit 06a669b into scalameta:master Mar 23, 2020

gabro deleted the new-file-action branch March 23, 2020 16:08

ckipp01 mentioned this pull request Mar 25, 2020

Create new symbol code action not working scalameta/coc-metals#134

Closed

This was referenced Apr 10, 2020

Create new symbol doesn't open new file after creation #1599

Closed

Listen to metals/executeClientCommand and handle metals-goto-location scalameta/metals-sublime#16

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "Create new symbol '$name'..." code action #1528

Add "Create new symbol '$name'..." code action #1528

gabro commented Mar 23, 2020

gabro Mar 23, 2020

gabro Mar 23, 2020

gabro Mar 23, 2020

olafurpg left a comment

olafurpg Mar 23, 2020

gabro Mar 23, 2020

olafurpg Mar 23, 2020 •

edited

Loading

gabro Mar 23, 2020

gabro Mar 23, 2020

olafurpg Mar 23, 2020

gabro Mar 23, 2020

tgodzik left a comment

gabro Mar 23, 2020

olafurpg Mar 23, 2020

ckipp01 Mar 23, 2020

tgodzik left a comment

ckipp01 left a comment

ckipp01 Mar 23, 2020

gabro commented Mar 23, 2020 •

edited

Loading

kpbochenek commented Mar 23, 2020

gabro commented Mar 23, 2020

gabro commented Mar 23, 2020

Add "Create new symbol '$name'..." code action #1528

Add "Create new symbol '$name'..." code action #1528

Conversation

gabro commented Mar 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

ckipp01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabro commented Mar 23, 2020 • edited Loading

kpbochenek commented Mar 23, 2020

gabro commented Mar 23, 2020

gabro commented Mar 23, 2020

olafurpg Mar 23, 2020 •

edited

Loading

gabro commented Mar 23, 2020 •

edited

Loading