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

fix: version 2022.3 breaks by marking PreloadingActivity for internal usage #107

Merged
merged 1 commit into from
May 10, 2023

Conversation

TomerFi
Copy link
Collaborator

@TomerFi TomerFi commented Mar 26, 2023

Fixes #35, Fixes #76, Fixes #81, Fixes #91, Fixes #92

Tested locally with versions:

  • 2021.1 (our minimum target)
  • 2022.2
  • 2022.3 (the latest version)

Edit 1:
Looks like this fix, though fixed the issue at hand, introduced a new one.
With this fix, diagnostic is only performed for modifying an already open file, and not for opening one.

Edit 2:
The cause for the error described in Edit 1 is lsp4intellij.
This bug was introduced in version 0.95.1, and can be demonstrated using the master-snapshot as well.
However, pinning the version to 0.95.0 resolves this issue.
Pinning the version is handled in #106.

@github-advanced-security
Copy link

You have successfully added a new SonarCloud configuration ``. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@TomerFi
Copy link
Collaborator Author

TomerFi commented Mar 27, 2023

/hold

@TomerFi
Copy link
Collaborator Author

TomerFi commented Mar 27, 2023

/unhold

Copy link
Member

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

Dependency changes should be in a separate PR

@TomerFi
Copy link
Collaborator Author

TomerFi commented Mar 27, 2023

Dependency changes should be in a separate PR

@jeffmaury
#106

@TomerFi TomerFi requested a review from jeffmaury March 27, 2023 08:45
@TomerFi TomerFi force-pushed the fix-2022-3-break branch 3 times, most recently from 3f6ba37 to ec2f616 Compare March 28, 2023 06:56
@TomerFi
Copy link
Collaborator Author

TomerFi commented Mar 28, 2023

rebased from the master after previous prs merged.

Copy link
Member

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

Seems to be several concerns are mixed in this PR

@TomerFi
Copy link
Collaborator Author

TomerFi commented Mar 28, 2023

Seems to be several concerns are mixed in this PR

@jeffmaury
First of all, thank you for your review.
Secondly, if there are multiple concerns here, it's because they are entangled with each other.

Background
This project uses lsp4intellij@master-SNAPSHOT (see #106).
This projects latest release (0.5.0) was on January 16th.
The README for lsp4intellij on that date looked something like this, which matches this plugin configuration at the time of the release build.

Today, the configuration is different, not only is it now used as a application service instead of the deprecated application component, but the extensions configuration is completely different now. You can check this compare link and pay extra attention to the original README.md vs. the current plugin.xml.example.

Initial Issue
As explained in #91, IntelliJ 2022.3 broke the the way we download the executables.
To fix that, I switched the usage of the broken Preload Activity to a more stable approach using the Application Lifecycle Listener.

This seems to do the trick, now the executables were downloaded as expected, and I could verify stack analysis was working again.

But... I couldn't verify component analysis because, as mentioned in the background, we're using a SNAPSHOT lsp4intellij and its configuration was changed dramatically from the last build done for this plugin's 0.5.0 release.
So I adapted the configuration accordingly and pushed this PR.

Happy Side Effect
As I was verifying our fix works with the adapted configuration of lsp4intellij, I was also able to verify that this configuration adaptation seems to also resolves #35, #76, #81, and #92.

@TomerFi TomerFi requested a review from jeffmaury March 28, 2023 14:36
@jeffmaury
Copy link
Member

I see another PR for updating the lsp4intellij dependency, so:

  • are the changes related to lsp4intellij in this PR relevant in the other PR and in that case they should be in this PR
  • if not they should be removed and this PR rebased after the other PR has been merged

@TomerFi
Copy link
Collaborator Author

TomerFi commented Mar 28, 2023

@jeffmaury

I see another PR for updating the lsp4intellij dependency, so:

Yeah, you mean #106 which I opened based on one of your requests.

  • are the changes related to lsp4intellij in this PR relevant in the other PR and in that case they should be in this PR

They were in this PR but you asked me to take it out:
image

  • if not they should be removed and this PR rebased after the other PR has been merged

They are not related, #106 is meant to fix another issue caused by a bad practice used in this plugin's design.

i.e. using unstable snapshot versions made us oblivious to the fact that lsp4intellij version 0.95.1 broke diagnostics for opening files (works only for modifying files), hence #106 is pinning lsp4intellij to the last known working version 0.95.0 instead of the unstable snapshot version used today.

@TomerFi TomerFi mentioned this pull request Apr 12, 2023
@TomerFi
Copy link
Collaborator Author

TomerFi commented Apr 14, 2023

Rebased.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Collaborator

@ruromero ruromero left a comment

Choose a reason for hiding this comment

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

LGTM

@ruromero
Copy link
Collaborator

ruromero commented May 8, 2023

@jeffmaury do you have any further comments? If not we will merge it.
Thanks in advance.

@ruromero ruromero merged commit 89f0656 into redhat-developer:main May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants