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

Keep code lenses after error until next successful compilation #994

Merged

Conversation

@marek1840
Copy link
Collaborator

marek1840 commented Oct 16, 2019

Closes #992
Previously, code lens would not disappear when recompilation would fail.
Now, we refresh the code lenses in an open tab also after failure

@marek1840 marek1840 force-pushed the marek1840:invalidate-code-lenses-after-error branch from 15bf8b3 to 2c77f4e Oct 16, 2019
)
_ <- assertCodeLenses(
"a/src/main/scala/Main.scala",
"""object Main {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 16, 2019

Member

Do I understand correctly that the code lens is removed due to the compile error?

I'm not sure this is desirable behavior. I would expect the code lens to stay stable until a successful compilation invalidates the main/test class.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 16, 2019

Member

Reasoning: I would like the "run" and "test" code lenses to be as stable as possible to avoid jittery code lenses. If the user requests to run a main or test on broken code then we could do a window/showMessage or something like that to explain that it's not possible to run the code because it's not compiling.

This comment has been minimized.

Copy link
@marek1840

marek1840 Oct 16, 2019

Author Collaborator

I can see a value in that and it is consistent with the approach "if it doesn't compile, first - make it compile"

@marek1840 marek1840 marked this pull request as ready for review Oct 16, 2019
@marek1840 marek1840 requested review from olafurpg and tgodzik Oct 16, 2019
@marek1840 marek1840 changed the title Refresh code lenses after error Keep code lenses after error until next successful compilation Oct 16, 2019
Copy link
Collaborator

tgodzik left a comment

A couple of small comments, but otherwise LGTM

@marek1840 marek1840 force-pushed the marek1840:invalidate-code-lenses-after-error branch 5 times, most recently from 1f439b1 to fb4b304 Oct 17, 2019
@marek1840

This comment has been minimized.

Copy link
Collaborator Author

marek1840 commented Oct 18, 2019

Other tests are still flaky but at least those related to code lenses seem to be fixed

@@ -85,6 +86,13 @@ abstract class BaseLspSuite(suiteName: String) extends BaseSuite {
.resolve("e2e")
.resolve(suiteName)
.resolve(name.replace(' ', '-'))

if (path.isDirectory) {

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 18, 2019

Collaborator

Isn't this done in cleanWorkspace?

This comment has been minimized.

Copy link
@marek1840

marek1840 Oct 18, 2019

Author Collaborator

Right, I will inline it in here


object TestingClient {
trait Listener {
def onRefreshModel(): Unit

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 18, 2019

Collaborator

Is this used?

marek1840 and others added 5 commits Oct 15, 2019
Also:
a) remove promises from build target classes
Since we make sure to force the codeLens request after compilation,
there is no need to use a promise - we have a guarantee that after
the refreshModel notification is sent to the client, the classes are
already up to date

b) emove workspace after each lsp test
Otherwise, running a test once could have an impact on consecutive runs
(try CodeLensLspSuite without this change - bloop analysis from previous
compilation is affecting the results)
@tgodzik tgodzik force-pushed the marek1840:invalidate-code-lenses-after-error branch from 7006988 to aaf364f Oct 18, 2019
Copy link
Member

olafurpg left a comment

LGTM 👍 It's great to formalize in tests the expected behavior. Thank you @marek1840 !

@olafurpg olafurpg merged commit a5050d7 into scalameta:master Oct 18, 2019
3 checks passed
3 checks passed
build
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
scalameta.metals Build #20191018.21 succeeded
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.