Skip to content

fix: publish BSP diagnostics when it's necessary #6929

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
Merged
merged 2 commits into from
Jun 25, 2022
Merged

fix: publish BSP diagnostics when it's necessary #6929

merged 2 commits into from
Jun 25, 2022

Conversation

kpodsiad
Copy link
Contributor

  • do not publish bsp diagnostics if there were and there are no problems
  • publish diagnostics if problems needs to be updated

continues effort of #6847, closes #6896

@adpi2 do you have an idea how to write test for this PR which will cover #6896?

kpodsiad
kpodsiad commented Jun 17, 2022
View reviewed changes
@adpi2
Copy link
Member

adpi2 commented Jun 17, 2022

@kpodsiad You can add a test in https://github.com/sbt/sbt/blob/1.7.x/server-test/src/test/scala/testpkg/BuildServerTest.scala

If you need to change a file in the project, to have different diagnostics you can do something like that:

sbt/server-test/src/test/scala/testpkg/BuildServerTest.scala

Lines 256 to 263 in f816aed

IO.write(
otherBuildFile,
"""
|val someSettings = Seq(
| scalacOptions += "-deprecation"
|)
|""".stripMargin
)

@adpi2 adpi2 self-requested a review June 17, 2022 08:08
asflierl
asflierl reviewed Jun 20, 2022
View reviewed changes
adpi2
adpi2 reviewed Jun 20, 2022
View reviewed changes
@kpodsiad kpodsiad force-pushed the bsp-diagnostics-fix branch 6 times, most recently from 3a0dfe3 to 4730317 Compare June 22, 2022 07:22
adpi2
adpi2 reviewed Jun 22, 2022
View reviewed changes
Copy link
Member

@adpi2 adpi2 left a comment

Choose a reason for hiding this comment

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

There is still one bug with the warnings. Also I think there are some more reports that could be avoided.

@kpodsiad
Copy link
Contributor Author

kpodsiad commented Jun 23, 2022 •
edited
Loading

@adpi2 I found interesting behavior (it's already present, tested on 1.6.2). Lets have files A and B.

  • A has a warning
  • make error in B
  • warning in A disappears
  • fix error in B
  • warning in A occurs again

Not sure if it's a bug or feature, however Bloop doesn't clear the warning.

@kpodsiad
fix: publishing bsp diagnostics
e8d60ef 
* do not publish bsp diagnostics if there were and there are no problems
* publish diagnostics if problems needs to be updated
@kpodsiad kpodsiad force-pushed the bsp-diagnostics-fix branch from 4730317 to e8d60ef Compare June 23, 2022 06:25
adpi2
adpi2 approved these changes Jun 24, 2022
View reviewed changes
Copy link
Member

@adpi2 adpi2 left a comment

Choose a reason for hiding this comment

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

Thanks @kpodsiad for fixing this! I also appreciate the refactoring in the BuildServerTest.

You are right about the strange behavior of warnings that disappear and then re-appear again. This is a small bug that is not critical. We will probably be able to fix it by storing more information about previous compilation in the BspCompileState class. I suggest we do that in a separate PR.

@adpi2 adpi2 merged commit a2060c9 into sbt:develop Jun 25, 2022
@adpi2 adpi2 mentioned this pull request Jun 25, 2022
Merged
eed3si9n added a commit that referenced this pull request Jun 25, 2022
@eed3si9n
Merge pull request #6933 from adpi2/backport-6929
2fc446b 
[1.7.x] Backport #6929: Fix publishing BSP diagnostics
@eed3si9n eed3si9n mentioned this pull request Jun 26, 2022
Closed
@tanishiking tanishiking mentioned this pull request Jul 7, 2022
Closed
@eed3si9n eed3si9n mentioned this pull request Jul 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adpi2 adpi2 adpi2 approved these changes

+1 more reviewer

@asflierl asflierl asflierl left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kpodsiad @adpi2 @asflierl