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

feat: implement Convert to collect code action #6969

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

scarf005
Copy link
Contributor

@scarf005 scarf005 commented Nov 23, 2024

collect.webm

resolves: scalameta/metals-feature-requests#288

@scarf005 scarf005 changed the title feat: convert filter + map to collect feat: implement Convert to collect code action Nov 23, 2024
@scarf005 scarf005 force-pushed the feat/collect-code-action branch 4 times, most recently from c542d5a to 6b4d8c9 Compare November 23, 2024 20:12
@scarf005 scarf005 force-pushed the feat/collect-code-action branch from 6b4d8c9 to f4d2a4a Compare November 25, 2024 13:22
@scarf005 scarf005 requested a review from tgodzik November 25, 2024 13:25
@scarf005 scarf005 force-pushed the feat/collect-code-action branch from f4d2a4a to 87fe644 Compare November 27, 2024 00:59
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I would want to improve is to make sure that the collect actually exist, for that I think we could reshuffle things a bit and use codeAction/resolve.

I haven't added it yet, but I can:

  • change your PR to use codeAction/resolve and only verify that collect exists on resolving
  • add the codeAction/resolve separately to use this week and let you use it
  • if you are interested I can explain exactly what can be done and let you work on that

What do you think?

Overall, I think we can check the symbol with semanticdb and the check for collect using compilers.info() <- if that returns some sensible information we should be fine.

There is also an option of rewriting it to the presentation compiler, but that's a much bigger rewrite.

What do you think?

@scarf005
Copy link
Contributor Author

sorry for late response, i accidentally marked notification to this as done.

i'm fine with either approach; (to be honest, doesn't know which way is the best)

if you are interested I can explain exactly what can be done and let you work on that

that would be great!

@scarf005 scarf005 force-pushed the feat/collect-code-action branch from 87fe644 to b0d38bb Compare November 27, 2024 12:47
@tgodzik
Copy link
Contributor

tgodzik commented Nov 27, 2024

I will try to merge #6978 tomorrow, but it adds the possibility of using codeAction/resolve.

We can add something similar to FilterMapChain class to data and no edits. If not edits are provided codeAction/resolve will be invoked, where we can additionally check if collect exists by:

  1. First send go to compilers.definition on filter or map
  2. Replace last map or filter with collect in the semanticdb symbol
  3. Use compilers.info to see if that symbol exists and if it exists if it's in an expected shape.
  4. If that's known we just create the edits.

@tgodzik
Copy link
Contributor

tgodzik commented Nov 28, 2024

Ok, the other PR is merged, let me know if you have any problems with what I suggested, I can also help finish the PR

@scarf005
Copy link
Contributor Author

currently stuck on NPE while converting to codeAction/resolve.

==> X tests.codeactions.FilterMapToCollectCodeActionSuite.cursor-on-filter  4.928s java.lang.NullPointerException: Cannot invoke "org.eclipse.lsp4j.Range.getStart()" because "range$1" is null

let me know if you have any problems with what I suggested, I can also help finish the PR

I'd be grateful!

@scarf005 scarf005 force-pushed the feat/collect-code-action branch from b0d38bb to 5c4d2a7 Compare November 30, 2024 06:46
@scarf005
Copy link
Contributor Author

scarf005 commented Nov 30, 2024

@tgodzik i've rebased and migrated to codeAction/resolve. still stuck on semanticDB part.

strangely, while it doesn't happen on my local machine the NPE still seems to happen in CI:

tests.codeactions.FilterMapToCollectCodeActionSuite:
  + cursor-on-filter 8.24s
  + cursor-on-filter-newline 2.663s
  + cursor-on-map 2.07s
  + higher-order-function 1.943s
  + higher-order-function-rename-filter-argument 1.924s
  + higher-order-function-rename-map-argument 1.516s
  + multiple-map-calls 1.744s
  + complex-predicate 1.779s
  + multiline-predicate 1.406s
  + multiline-predicate-block 1.843s
  + with-type-annotations 1.632s
[info] Passed: Total 11, Failed 0, Errors 0, Passed 11

https://github.com/scalameta/metals/actions/runs/12094357358/job/33726319967?pr=6969#step:5:1816
image

override def resolveCodeAction(codeAction: l.CodeAction, token: CancelToken)(
implicit ec: ExecutionContext
): Option[Future[l.CodeAction]] = {
println(codeAction.getData.toJson)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if data is null then this will throw an exception

): Option[Future[l.CodeAction]] = {
println(codeAction.getData.toJson)
val edits = for {
data <- codeAction.getData.toJson.as[FilterMapCollectParams].toOption
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
data <- codeAction.getData.toJson.as[FilterMapCollectParams].toOption
data <- parseData[FilterMapCollectParams](codeAction)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add filter+map to collect code action
2 participants