Client connector migrated to java #264

Merged
merged 15 commits into from Nov 14, 2016

Projects

None yet

4 participants

@hendrikebbers
Member
hendrikebbers commented Oct 26, 2016 edited

For several issues we need to refactor the client connector. This PR migrates it to Java for a better refactoring.


This change is Reviewable

hendrikebbers added some commits Oct 25, 2016
@hendrikebbers hendrikebbers externalized command handling 123304f
@hendrikebbers hendrikebbers Logger ab7bb92
@hendrikebbers hendrikebbers exception handler 78a6259
@hendrikebbers hendrikebbers Java style method calls 0b4cbb7
@hendrikebbers hendrikebbers Java style 01548da
@hendrikebbers hendrikebbers access modifiers e642bd3
@hendrikebbers hendrikebbers access modifiers
32a2e13
@hendrikebbers hendrikebbers command handling
654670b
@hendrikebbers hendrikebbers Groovy command handling
39a3e84
@hendrikebbers hendrikebbers modifiers
89114d0
@hendrikebbers hendrikebbers additional classes converted
165410a
@hendrikebbers hendrikebbers added this to the 0.8.8 milestone Oct 26, 2016
@hendrikebbers hendrikebbers header
d31b54c
@coveralls

Coverage Status

Coverage increased (+0.2%) to 68.407% when pulling d31b54c on connector-to-java into b7e6780 on master.

@coveralls

Coverage Status

Coverage increased (+0.2%) to 68.407% when pulling d31b54c on connector-to-java into b7e6780 on master.

@hendrikebbers hendrikebbers changed the title from Remting client connector migrated to java to Client connector migrated to java Oct 27, 2016
@blackdrag

Reviewed 12 of 14 files at r1.
Review status: 10 of 12 files reviewed at latest revision, 3 unresolved discussions.


remoting/dolphin-remoting-client/src/main/groovy/org/opendolphin/core/client/comm/AbstractClientConnector.java, line 80 at r1 (raw file):

    public AbstractClientConnector(ClientDolphin clientDolphin, ICommandBatcher commandBatcher) {
        this.clientDolphin = clientDolphin;
        this.commandBatcher = DefaultGroovyMethods.asBoolean(commandBatcher) ? commandBatcher : new CommandBatcher();

I think you can replace this with a null check


remoting/dolphin-remoting-client/src/main/groovy/org/opendolphin/core/client/comm/CommandAndHandler.java, line 41 at r1 (raw file):

     */
    public boolean isBatchable() {
        if (DefaultGroovyMethods.asBoolean(handler)) return false;

I think this can be a null check


remoting/dolphin-remoting-client/src/test/groovy/org/opendolphin/core/client/comm/ClientConnectorTests.groovy, line 113 at r1 (raw file):

  //  }
  //  assert msg == exceptionMessage
  //}

why is this test commented out?


Comments from Reviewable

@blackdrag

Reviewed 1 of 14 files at r1.
Review status: 11 of 12 files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@hendrikebbers
Member

Review status: 11 of 12 files reviewed at latest revision, 3 unresolved discussions.


remoting/dolphin-remoting-client/src/main/groovy/org/opendolphin/core/client/comm/AbstractClientConnector.java, line 80 at r1 (raw file):

Previously, blackdrag (Jochen Theodorou) wrote…

I think you can replace this with a null check

done

remoting/dolphin-remoting-client/src/main/groovy/org/opendolphin/core/client/comm/CommandAndHandler.java, line 41 at r1 (raw file):

Previously, blackdrag (Jochen Theodorou) wrote…

I think this can be a null check

done

remoting/dolphin-remoting-client/src/test/groovy/org/opendolphin/core/client/comm/ClientConnectorTests.groovy, line 113 at r1 (raw file):

Previously, blackdrag (Jochen Theodorou) wrote…

why is this test commented out?

Because the exception handler has now String return value anymore. This was only used in this test. Everything else do not handle a result. Based on this the method is void now.

Comments from Reviewable

hendrikebbers added some commits Nov 14, 2016
@hendrikebbers hendrikebbers Based on review
b5d8a8d
@hendrikebbers hendrikebbers Based on review
e586e14
@coveralls

Coverage Status

Coverage increased (+0.4%) to 68.603% when pulling e586e14 on connector-to-java into b7e6780 on master.

@hendrikebbers hendrikebbers more wait time since test fails on travis
6b43729
@coveralls

Coverage Status

Coverage increased (+0.4%) to 68.603% when pulling 6b43729 on connector-to-java into b7e6780 on master.

@hendrikebbers hendrikebbers merged commit ada5c8d into master Nov 14, 2016

3 of 4 checks passed

code-review/reviewable 4 files, 3 discussions left (blackdrag)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.4%) to 68.603%
Details
@aalmiray aalmiray removed the in progress label Nov 14, 2016
@blackdrag

remoting/dolphin-remoting-client/src/test/groovy/org/opendolphin/core/client/comm/ClientConnectorTests.groovy, line 113 at r1 (raw file):

Previously, hendrikebbers (Hendrik Ebbers) wrote…

Because the exception handler has now String return value anymore. This was only used in this test. Everything else do not handle a result. Based on this the method is void now.

one of use is misunderstanding this test. onException is supposed to be called with an exception to then throw it in the appropriate context or to throw it. In this test the exception is thrown and catched by the shouldFail-method (part of GroovyTestCase, not dolphin) and then return the exception message. If the exception does not appear, you changed the threads this test runs in and what the UI thread is, or you now catch the exception at another place and you have to think about that is really intended. Since you set the ui thread handler here, I cannot imagine it is related to threading.

Comments from Reviewable

@hendrikebbers
Member

remoting/dolphin-remoting-client/src/test/groovy/org/opendolphin/core/client/comm/ClientConnectorTests.groovy, line 113 at r1 (raw file):

Previously, blackdrag (Jochen Theodorou) wrote…

one of use is misunderstanding this test. onException is supposed to be called with an exception to then throw it in the appropriate context or to throw it. In this test the exception is thrown and catched by the shouldFail-method (part of GroovyTestCase, not dolphin) and then return the exception message. If the exception does not appear, you changed the threads this test runs in and what the UI thread is, or you now catch the exception at another place and you have to think about that is really intended. Since you set the ui thread handler here, I cannot imagine it is related to threading.

ok, since it is already merged I will add an issue

Comments from Reviewable

@hendrikebbers hendrikebbers deleted the connector-to-java branch Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment