-
Notifications
You must be signed in to change notification settings - Fork 5
fix: stop workspace if server process was stopped (#23579) #203
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #203 +/- ##
========================================
+ Coverage 0.00% 6.58% +6.58%
========================================
Files 4 32 +28
Lines 26 1184 +1158
Branches 0 215 +215
========================================
+ Hits 0 78 +78
- Misses 26 1098 +1072
- Partials 0 8 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a0834ee to
e7c2392
Compare
|
|
||
| delay(500L) | ||
| } | ||
| true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code here.
Otherwise looks good and reasonable to me.
If/when merged I'll have to align #205 according to this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vrubezhny you're absolutely correct but it wont compile without it because the method has a boolean return value. I dont see any way to have it compile without the dead code. I'll add a comment that explains this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, we can make it like:
@Suppress("UNREACHABLE_CODE")
null
I understand that this actually is not reachable code, but also returning true doesn't seem a correct thing...
It makes a fake impression of successful result, which isn't true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, doing
842f40b to
a2e1967
Compare
|
@vrubezhny, @msivasubramaniaan: rebased it, can you please test/review it again? It should still enable/disable the connect button. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks!
|
|
||
| delay(500L) | ||
| } | ||
| true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, we can make it like:
@Suppress("UNREACHABLE_CODE")
null
I understand that this actually is not reachable code, but also returning true doesn't seem a correct thing...
It makes a fake impression of successful result, which isn't true.
| import com.jetbrains.gateway.thinClientLink.LinkedClientManager | ||
| import com.jetbrains.gateway.thinClientLink.ThinClientHandle | ||
| import com.jetbrains.rd.util.lifetime.Lifetime | ||
| import com.redhat.devtools.gateway.openshift.DevWorkspace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import isn't really used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
Signed-off-by: Andre Dietisheim <adietish@redhat.com>
Signed-off-by: Andre Dietisheim <adietish@redhat.com> Generated-by: gemini-cli
|
fixed & rebased |
fixes eclipse-che/che#23579