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

Report an issue only if SemanticDB file doesn't really exist. #853

Merged
merged 1 commit into from Aug 6, 2019

Conversation

@tgodzik
Copy link
Collaborator

commented Aug 5, 2019

Previously we would report that SemanticDB file does not exist in case when symbol was correctly not found (Ctrl on an empty place), now we check if SemanticDB really exists.

…n symbol was correctly not found (Ctrl on an empty place), now we check if SemanticDB really exists
@tgodzik tgodzik requested a review from marek1840 Aug 5, 2019
Copy link
Collaborator

left a comment

Looks good. I have left a suggestion, if you want to improve (?) on this a bit.

@@ -1583,7 +1585,9 @@ class MetalsLanguageServer(
definitionResult(position, token)
}
case None =>
warnings.noSemanticdb(source)
if (semanticDBDoc.isEmpty) {

This comment has been minimized.

Copy link
@marek1840

marek1840 Aug 6, 2019

Collaborator

or we could do:

val semanticDBDoc = semanticdbs.textDocument(source).documentIncludingStale
if(semanticDBDoc.isEmpty){
  warnings.noSemanticdb(source)
} else {
  val occurrence = for { ...} yield occ
  occurrence match {
    case None =>
      // ignore
    case Some(occ) =>
      (...)
  }
}

would probably make the logic simpler (no need to remember about semanticDBDoc being emoty or not when working on occurrences).

or even something like that:

val semanticDBDoc = semanticdbs.textDocument(source).documentIncludingStale 
semanticDBDoc match {
  case None => warnings.noSemanticdb(source)
  case Some(doc) =>
    val occurrence = definitionProvider.positionOccurrence(source, position, doc).occurrence 
    occurrence.foreach { occ =>
        (...)
    }

This comment has been minimized.

Copy link
@tgodzik

tgodzik Aug 6, 2019

Author Collaborator

I'd rather not change too much now.

@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 6, 2019

Failures do not look related. merging.

@tgodzik tgodzik merged commit 3247765 into scalameta:master Aug 6, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
scalameta.metals Build #20190805.2 failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tgodzik tgodzik deleted the tgodzik:fix-wrong-warning branch Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.