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

Enable goToDefinition when cursor is at end of symbol #965

Merged
merged 1 commit into from Oct 8, 2019

Conversation

@ashwinbhaskar
Copy link
Contributor

ashwinbhaskar commented Oct 6, 2019

Fixes #945

@tgodzik
tgodzik approved these changes Oct 6, 2019
Copy link
Collaborator

tgodzik left a comment

Thanks for the contribution!

Copy link
Member

olafurpg left a comment

Thank you for this contribution!

@ashwinbhaskar ashwinbhaskar force-pushed the ashwinbhaskar:goto_definition_fix branch 2 times, most recently from 4cb3245 to d222d54 Oct 6, 2019
@ashwinbhaskar ashwinbhaskar requested a review from olafurpg Oct 6, 2019
@ashwinbhaskar ashwinbhaskar force-pushed the ashwinbhaskar:goto_definition_fix branch from d222d54 to bffdf96 Oct 6, 2019
@ashwinbhaskar

This comment has been minimized.

Copy link
Contributor Author

ashwinbhaskar commented Oct 6, 2019

@olafurpg I am not able to see your requested changes. Probably because I forced pushed before reading it. Can you review it once more? Sorry for the trouble

Copy link
Collaborator

tgodzik left a comment

Thanks for fixing the test! I think we can remove the config option, sorry for the confusion.

@@ -119,7 +120,8 @@ object MetalsServerConfig {
statusBar = StatusBarConfig.logMessage,
// Not strictly needed, but helpful while this integration matures.
isHttpEnabled = true,
icons = Icons.unicode
icons = Icons.unicode,
includeLastCharacter = false

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 7, 2019

Collaborator

I think we can remove it from here and from config.

@@ -89,7 +89,7 @@ final class DefinitionProvider(
val occurrence = for {
queryPosition <- snapshotPosition.toPosition(dirtyPosition.getPosition)
occurrence <- snapshot.occurrences
.find(_.encloses(queryPosition, includeLastCharacter))
.find(_.encloses(queryPosition, config.includeLastCharacter))

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 7, 2019

Collaborator

we can set it to true here ande remove from config since we don't plan to have different behaviour per client.

@ashwinbhaskar ashwinbhaskar force-pushed the ashwinbhaskar:goto_definition_fix branch from bffdf96 to 44fd4ca Oct 7, 2019
@tgodzik
tgodzik approved these changes Oct 7, 2019
Copy link
Collaborator

tgodzik left a comment

Thanks! LGTM

Copy link
Member

olafurpg left a comment

LGTM 👍 Thank you for this contribution!

@olafurpg olafurpg merged commit 2a0ddd3 into scalameta:master Oct 8, 2019
2 checks passed
2 checks passed
build
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.