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

Update `isCompiled` status of the build target regardless of the compilation result #907

Conversation

@marek1840
Copy link
Collaborator

commented Sep 9, 2019

Previously, build target was removed from the isCompiled set only when the compilation finished successfully.

Now, we clear this set after every compilation.
Also, the lastCompile semantics remains unchanged: it contains only the build targets which were previously compiled successfully.

Previously, build target was removed from the `isCompiled` set
only when the compilation finished successfully.

Now, we clear this set after every compilation.
Also, the `lastCompile` semantics remains unchanged:
it contains only the build targets which were previously
compiled successfully
@marek1840 marek1840 requested a review from tgodzik Sep 9, 2019
@olafurpg

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Can you elaborate on the motivation for the change?

@tgodzik
tgodzik approved these changes Sep 9, 2019
Copy link
Collaborator

left a comment

Just a small nitpick, you can disregard.

import scala.collection.concurrent.TrieMap
import scala.concurrent.ExecutionContext
import scala.concurrent.Future

This comment has been minimized.

Copy link
@tgodzik

tgodzik Sep 9, 2019

Collaborator

Let's not add new newlines maybe?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Sep 10, 2019

Member

You can configure IntelliJ to never group imports and sort them alphabetically

@tgodzik

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

Can you elaborate on the motivation for the change?

Seems we were not clearing isCompiling status when we finished compilation with an error.

@marek1840

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

Can you elaborate on the motivation for the change?

Basically, I like when the names reflect the reality. In this case, we could either rename the isCompiling to isCompilingOrFailed or change the logic to reflect the name. Unless you know any reason to keep the logic, I would like to merge this change.

Copy link
Member

left a comment

Is this a semantic change or a refactoring on the variable name? If it's a semantic change, then it would be good to know what implications it can have on the user experience.

import scala.collection.concurrent.TrieMap
import scala.concurrent.ExecutionContext
import scala.concurrent.Future

This comment has been minimized.

Copy link
@olafurpg

olafurpg Sep 10, 2019

Member

You can configure IntelliJ to never group imports and sort them alphabetically

lastCompile = isCompiling.keySet
val compilation = connection
.compile(params)
.whenComplete((_, error) => {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Sep 10, 2019

Member

Let's try to use Scala's Future operations over CompletableFuture. It's easy to mess up with the execution context in the Java API in my experience and it's not idiomatic to for example guard against error == null

@marek1840

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 12, 2019

it would be good to know what implications it can have on the user experience

What get's affected:

  • this warning asking the user to wait won't show if the user has nothing to wait for.
  • the compilation on didFocus wouldn't trigger another compilation if the previous one failed but that should be a non-issue as we expect the user to fix the failures and save, triggering a compilation.
Copy link
Member

left a comment

@marek1840 Sounds great! Thanks for the explanation, I have observed both UX bugs 👍

@olafurpg olafurpg referenced this pull request Sep 12, 2019
@olafurpg olafurpg changed the title update `isCompiled` status of the build target regardless of the compilation result Update `isCompiled` status of the build target regardless of the compilation result Sep 12, 2019
@olafurpg

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Feel free to ignoret the flaky CI, I opened #918 to disable the failing test (which I have seen fail frequently in CI).

@tgodzik tgodzik merged commit 6ca76e5 into scalameta:master Sep 12, 2019
1 of 2 checks passed
1 of 2 checks passed
scalameta.metals Build #20190912.2 failed
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.