metals-feature-requests icon indicating copy to clipboard operation
metals-feature-requests copied to clipboard

Create new symbol action should allow you to create a method

Open ckipp01 opened this issue 3 years ago • 18 comments

Is your feature request related to a problem? Please describe.

Sometimes when I'm writing code I'm in this situation:

def doThing(a: Int, b: Int) = {
  doOtherStuff(a) // this will error because `doOtherStuff` doesn't exist
}

In this situations you can trigger a code action for doOtherStuff and it will show you the "Create new symbol 'doOtherStuff'" action. However, if you select that all the options are creating another file or a companion object, whereas I want doOtherStuff to be a method.

Describe the solution you'd like

I'd like another option to be offered, maybe just "method". And if I select it, the code I'd end up with is:

def doOtherStuff(a: Int) = ???

def doThing(a: Int, b: Int) = {
  doOtherStuff(a)
}

Describe alternatives you've considered

Doing this by hand

Additional context

No response

Search terms

create method

ckipp01 avatar Sep 01 '22 17:09 ckipp01

Thanks for reporting! I've actually been working on it in https://github.com/tgodzik/metals/tree/infer-method :sweat_smile:

tgodzik avatar Sep 02 '22 06:09 tgodzik

@tgodzik it seems your PR was at a good point: did you find a major obstacle to complete it? I was tempted to give a go at recovering it (just not very familiar with metals development, so I may need some guidance).

And a minor question: would it be much more effort to allow the following case as well?

before action:

class X {}

def doThing(a: Int, b: Int, x: X) = {
  x.doOtherStuff(a) // this will error because `doOtherStuff` doesn't exist
}

after action:

class X {
  def doOtherStuff(a: Int) = ???
}

def doThing(a: Int, b: Int, x: X) = {
  x.doOtherStuff(a)
}

ag91 avatar Oct 05 '24 23:10 ag91

I think some of the test might have not been passing, but don't remember why I never finished :thinking:

One thing that can better to do now is to use the codeAction method instead of adding a new one to the presentation compiler.

Feel, free to pick it up, the way I would go around it is:

  • check out the branch
  • rebase on the most recent main
  • try to run the tests via sbt cross/testOnly tests.pc.InsertInferredMethodSuite

You can add more tests there, they are pretty quick compared to full unit tests and offer quickest way to experiment. You can add println statements via pprint.log(...) and run a single test in watch mode to see what is going on sbt ~cross/testOnly tests.pc.InsertInferredMethodSuite -- name-of-the-test``

tgodzik avatar Oct 07 '24 14:10 tgodzik

Let me know if you have any more questions, I will try to answer them as soon as possible. I am also active on our discord.

tgodzik avatar Oct 07 '24 14:10 tgodzik

right, recovered the branch at https://github.com/ag91/metals/commits/infer-method. (BTW is it okay unrelated Hover tests are failing on main - at least on my local version)

It seems the scala 3 presentation compiler method is unimplemented: would using the codeAction method (do you mean this?) help with avoiding the repeated definition for each presentation compiler?

Also where could I get a gist of the architecture relevant to this change? (So I understand better what I am doing)

I had to comment out this test because it was failing (which is a good point to start feature work). Thanks for all the tests!

ag91 avatar Oct 07 '24 23:10 ag91

right, recovered the branch at https://github.com/ag91/metals/commits/infer-method. (BTW is it okay unrelated Hover tests are failing on main - at least on my local version)

This might be related to a JDK version, we test on JDK 17

It seems the scala 3 presentation compiler method is unimplemented: would using the codeAction method (do you mean this?) help with avoiding the repeated definition for each presentation compiler?

I don't think I ever managed to work on the Scala 3 support, it can be done separately. That's the codeAction method I mean. It's mostly to avoid having too many methods especially that some of them do exactly the same. And we don't have to later update interfaces in the Scala 3 compiler.

Also where could I get a gist of the architecture relevant to this change? (So I understand better what I am doing)

Code actions are implementations of CodeAction trait and they really have two phases. One is to identify that a code action is really possible there and then resolving it. Resolve is doing all the more intensive stuff and it's the gist of the change:

  • use an appropriate method in the Compilers which resolve the exact version of the presentation compiler we need
  • then it will use the specific method in the implementation of PresentationCompiler, where we need to put most of the logic and it's already partly done with the class I added.

code action request -> return all code action -> user chooses a code action -> resolve code action -> use the presentation compiler if needed -> return an edit to the editor to be applied

I hope it clears things a bit.

tgodzik avatar Oct 08 '24 08:10 tgodzik