-
Notifications
You must be signed in to change notification settings - Fork 2k
Initial implementation of Exec. #21
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
956dea2
to
7a5fed8
Compare
@mbohlool Now that websockets support is merged, I rebased this on top of that PR. Please take a look. Thanks! |
@mbohlool friendly ping on this if you have any feedback. Thanks! |
examples/pom.xml
Outdated
@@ -19,6 +19,13 @@ | |||
<artifactId>client-java-util</artifactId> | |||
<version>1.0-SNAPSHOT</version> | |||
</dependency> | |||
<!-- https://mvnrepository.com/artifact/com.google.guava/guava --> |
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.
indentation?
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.
fixed.
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.
I can still see the indentation problem here.
} | ||
|
||
public void destroy() { | ||
// TODO implementation here |
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.
unnecessary comment?
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.
Implemented :)
@@ -26,6 +26,8 @@ | |||
|
|||
import javax.net.ssl.KeyManager; | |||
|
|||
import org.apache.commons.codec.binary.Base64; |
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.
unused import. is there a way to detect these automatically in a github check? like a linter?
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.
I can't find a good one. Eclipse will do it, but then I'd have to use eclipse :)
|
||
import static java.util.concurrent.TimeUnit.SECONDS; | ||
|
||
public final class WebSocketCall { |
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.
where did you use this? I see one WebSocketCall in WebSockets.java but it is not using this class. it uses WebSocketCall in another package.
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.
Removed. (it was part of the GET vs. POST discussion)
public void onOpen(WebSocket webSocket, Response response) { | ||
listener.open(); | ||
public void onOpen(final WebSocket webSocket, Response response) { | ||
listener.open("todo", new Closeable() { |
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.
todo?
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.
Implemented. thanks for the reminder.
Comments addressed, please re-check. Thanks! |
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 except the indentation problem.
examples/pom.xml
Outdated
@@ -19,6 +19,13 @@ | |||
<artifactId>client-java-util</artifactId> | |||
<version>1.0-SNAPSHOT</version> | |||
</dependency> | |||
<!-- https://mvnrepository.com/artifact/com.google.guava/guava --> |
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.
I can still see the indentation problem here.
@mbohlool oops, sorry, I formatted the Thanks! |
Depends on #19
Fixes #9