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

improvement: clear diagnostics in downstream targets #6604

Merged
merged 6 commits into from
Aug 6, 2024

Conversation

kasiaMarek
Copy link
Contributor

@kasiaMarek kasiaMarek commented Jul 18, 2024

when compilation in a upstream target failed. We don't do it for Scala 3 best effort compilation, for which even if compilation fails upstream often downstream projects will also get recompiled.

resolves: #2670

@kasiaMarek kasiaMarek changed the title improvement: clear diagnostics in downstream targets [WIP] improvement: clear diagnostics in downstream targets Jul 18, 2024
@kasiaMarek kasiaMarek changed the title [WIP] improvement: clear diagnostics in downstream targets improvement: clear diagnostics in downstream targets Jul 22, 2024
@kasiaMarek kasiaMarek marked this pull request as ready for review July 22, 2024 08:27
@kasiaMarek kasiaMarek requested a review from tgodzik July 22, 2024 08:27
@@ -101,7 +105,7 @@ final class Compilations(
result <- targetOpt match {
case None => Future.successful(empty)
case Some(target) =>
compileBatch(target)
compileBatch(target) // HERE
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
compileBatch(target) // HERE
compileBatch(target)


import ch.epfl.scala.bsp4j.BuildTargetIdentifier

class BuildTargetMapper {
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
class BuildTargetMapper {
class PreviouslyCompiledTargets {

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not perfect, but I don't have a better name. I also added a comment, because the whole logic may not be that obvious. Can you check if it is understandable?

val targets = for {
path <- diagnostics.keySet
targets <- buildTargets.sourceBuildTargets(path)
if targets.forall(inverseDeps.apply)
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
if targets.forall(inverseDeps.apply)
if targets.exists(inverseDeps.apply)

?

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.

Just some minor comments left. Great work!

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.

LGTM!

@kasiaMarek kasiaMarek merged commit 074cf7a into scalameta:main Aug 6, 2024
21 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stale diagnostic in downstream targets
2 participants