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.

Sign up for GitHub

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

Jump to bottom

Expand Selection in comments for Scala 2 #6109

Merged
merged 19 commits into from
Apr 11, 2024
Merged

Expand Selection in comments for Scala 2 #6109

merged 19 commits into from
Apr 11, 2024

Conversation

doofin
Copy link
Contributor

@doofin doofin commented Feb 10, 2024 •
edited
Loading

fix #3615 for Scala 2

@doofin doofin marked this pull request as draft February 10, 2024 17:16
@doofin
Copy link
Contributor Author

doofin commented Feb 10, 2024

the tests are successful, but how do I manually test it? publishLocal;mtags/publishLocal doesn't seem to work and there's errors in metals log like:

Error downloading org.scalameta:mtags_2.12.18:1.2.1-SNAPSHOT

@doofin doofin marked this pull request as ready for review February 10, 2024 20:12
@kasiaMarek
Copy link
Contributor

kasiaMarek commented Feb 12, 2024 •
edited
Loading

the tests are successful, but how do I manually test it? publishLocal;mtags/publishLocal doesn't seem to work and there's errors in metals log like:
Error downloading org.scalameta:mtags_2.12.18:1.2.1-SNAPSHOT

I'm guessing you're referring to this: https://scalameta.org/metals/docs/contributors/getting-started/#manually-testing-an-lspsuite ?publishLocal; mtags/publishLocal will only publish locally mtags for Scala 2.13.12, you need to run publishLocal; ++2.12.18 mtags/publishLocal to publish for Scala 2.12.18. You can also run quick-publish-local, which will locally publish mtags for a few Scala versions (including 2.12.18).

@doofin
Copy link
Contributor Author

doofin commented Feb 14, 2024

@kasiaMarek thanks, no I want to test metals plugin it in vscode for 2.13 and 3, does quick-publish-local make it work for major versions like 2.12, 2.13, 3?

@kasiaMarek
Copy link
Contributor

does quick-publish-local make it work for major versions like 2.12, 2.13, 3

You can find all the quick publish versions here:

metals/project/V.scala

Line 136 in e65409f

val quickPublishScalaVersions =
but 2.12.18, 2.13.12, and 3.3.1 are included.

@doofin
Copy link
Contributor Author

doofin commented Feb 18, 2024 •
edited
Loading

thanks @kasiaMarek , able to test in 2.13 with ++2.13.12 publishLocal;mtags/publishLocal .

Now it's weird that expand Selection in comments doesn't seem to work in vscode for manual testing while other tests are successful. I print some logs for ranges and it looks correct . @tgodzik could you help me verify if it works on your side?

@doofin doofin mentioned this pull request Feb 19, 2024
Open
11 tasks
@doofin
Copy link
Contributor Author

doofin commented Feb 25, 2024 •
edited
Loading

seems like a bug in vscode extention from vscode Devtools:

ERR Cannot read properties of null (reading 'range'): TypeError: Cannot read properties of null (reading 'range')
	at asSelectionRange (/home/dhp/.vscode-oss/extensions/scalameta.metals-1.26.1/node_modules/vscode-languageclient/lib/common/protocolConverter.js:856:63)
	at convertBatch (/home/dhp/.vscode-oss/extensions/scalameta.metals-1.26.1/node_modules/vscode-languageclient/lib/common/utils/async.js:193:25)
	at Object.map (/home/dhp/.vscode-oss/extensions/scalameta.metals-1.26.1/node_modules/vscode-languageclient/lib/common/utils/async.js:202:17)
	at Object.asSelectionRanges (/home/dhp/.vscode-oss/extensions/scalameta.metals-1.26.1/node_modules/vscode-languageclient/lib/common/protocolConverter.js:862:22)
	at /home/dhp/.vscode-oss/extensions/scalameta.metals-1.26.1/node_modules/vscode-languageclient/lib/common/selectionRange.js:40:62
	at async ee.provideSelectionRanges (/usr/lib/code/out/vs/workbench/api/node/extensionHostProcess.js:150:108129)

related to vscode API provideSelectionRanges

@doofin
Copy link
Contributor Author

doofin commented Apr 5, 2024

@kasiaMarek @tgodzik shall we merge this first? unit test passed for this ,just that there's sth in the vscode plugin side that needs to be fixed

kasiaMarek
kasiaMarek reviewed Apr 5, 2024
View reviewed changes
Copy link
Contributor

@kasiaMarek kasiaMarek left a comment

Choose a reason for hiding this comment

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

Can you generally clean this up a bit? There are some printlns and commented code left.

tests/cross/src/test/scala/tests/pc/SelectionRangeCommentSuite.scala Outdated
Comment on lines 5 to 6
override def ignoreScalaVersion: Option[IgnoreScalaVersion] =
None // Some(IgnoreScala2)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's by default None so you can delete that whole thing.

mtags/src/main/scala-2/scala/meta/internal/pc/SelectionRangeProvider.scala Outdated
rg
}

def commentRangesFromTokens(
Copy link
Contributor

Choose a reason for hiding this comment

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

unused?

@doofin doofin requested a review from kasiaMarek April 6, 2024 22:29
@doofin
Copy link
Contributor Author

doofin commented Apr 6, 2024

thanks @kasiaMarek I cleaned them up, forget to do so previously

kasiaMarek
kasiaMarek approved these changes Apr 9, 2024
View reviewed changes
Copy link
Contributor

@kasiaMarek kasiaMarek left a comment

Choose a reason for hiding this comment

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

Just nitpicks, otherwise lgtm

mtags/src/main/scala/scala/meta/internal/pc/SelectionRangeUtils.scala
Comment on lines +24 to +32
.collect { case x: Comment =>
(x.start, x.end, x.pos)
}
.collect {
case (commentStart, commentEnd, _)
if commentStart <= cursorStartShifted && cursorStartShifted <= commentEnd =>
(commentStart + offsetStart, commentEnd + offsetStart)

}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can merge this into a single collect.

mtags/src/main/scala-2/scala/meta/internal/pc/SelectionRangeProvider.scala Outdated Show resolved Hide resolved
mtags/src/main/scala-2/scala/meta/internal/pc/SelectionRangeProvider.scala Outdated Show resolved Hide resolved
doofin and others added 2 commits April 10, 2024 13:49
@doofin
test
f3b833b
@doofin @kasiaMarek
Update mtags/src/main/scala-2/scala/meta/internal/pc/SelectionRangePr…
7426a4b 
…ovider.scala

Co-authored-by: Katarzyna Marek <kasia@marek.net>
@doofin
Copy link
Contributor Author

doofin commented Apr 10, 2024

thanks @kasiaMarek I fix some of the nits, but some other tests from main failed so I can't merge:

[error] Failed: Total 701, Failed 1, Errors 0, Passed 698, Skipped 2, Ignored 3
[error] Failed tests:
[error] 	tests.RenameLspSuite

@kasiaMarek kasiaMarek merged commit 08bcf0d into scalameta:main Apr 11, 2024
23 of 26 checks passed
@doofin doofin deleted the ExpandSelectionCommentScala2 branch April 11, 2024 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kasiaMarek kasiaMarek kasiaMarek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expand Selection selects too wide scope
2 participants
@doofin @kasiaMarek