Groovy to Java #270

Merged
merged 16 commits into from Nov 16, 2016

Projects

None yet

4 participants

@hendrikebbers
Member
hendrikebbers commented Nov 14, 2016 edited

Converted some Groovy classes to Java


This change is Reviewable

@hendrikebbers hendrikebbers added this to the 0.8.9 milestone Nov 14, 2016
@hendrikebbers hendrikebbers Merge branch 'master' into next-groovy-to-java
# Conflicts:
#	remoting/dolphin-remoting-client/src/main/groovy/org/opendolphin/core/client/comm/CommandAndHandler.java
#	remoting/dolphin-remoting-client/src/main/groovy/org/opendolphin/core/client/comm/CommandBatcher.groovy
#	remoting/dolphin-remoting-client/src/main/groovy/org/opendolphin/core/client/comm/ExceptionHandler.java
#	remoting/dolphin-remoting-client/src/main/groovy/org/opendolphin/core/client/comm/OnFinishedDataAdapter.java
#	remoting/dolphin-remoting-client/src/main/groovy/org/opendolphin/core/client/comm/RunLaterUiThreadHandler.java
8d37314
@coveralls

Coverage Status

Coverage increased (+0.5%) to 69.125% when pulling 8d37314 on next-groovy-to-java into 26c9664 on master.

@blackdrag

Reviewed 60 of 62 files at r1.
Review status: 60 of 62 files reviewed at latest revision, 3 unresolved discussions.


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

public class ClientAttribute extends BaseAttribute {
    /**
     * @deprecated you should not create Client Attributes without initial values

if this is deprecated and a breaking change why keep the method?


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

     */
    public ClientPresentationModel(String id, List<ClientAttribute> attributes) {
        super((id != null && StringGroovyMethods.asBoolean(id)) ? id : "" + instanceCount++ + AUTO_ID_SUFFIX, attributes);

ich würde das in eine statische Methode auslagern. Ausserdem würde ich id auf null und leeren String prüfen, statt asBoolean aufzurufen, Die Exception würde ich dann auch von der statischen Methode werfen.


remoting/dolphin-remoting-server/src/main/groovy/org/opendolphin/core/server/action/ClosureServerAction.java, line 15 at r1 (raw file):

    private final Closure namedCommandHandler;

    public ClosureServerAction(String name, Closure namedCommandHandler) {

Damit wirst du natürlich noch nicht die Groovy-Runtime los


Comments from Reviewable

@hendrikebbers
Member

Review status: 60 of 62 files reviewed at latest revision, 3 unresolved discussions.


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

Previously, blackdrag (Jochen Theodorou) wrote…

if this is deprecated and a breaking change why keep the method?

The method is still used at several places in the remoting layer. I would love to handle refactoring like this once all the classes are Java based.

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

Previously, blackdrag (Jochen Theodorou) wrote…

ich würde das in eine statische Methode auslagern. Ausserdem würde ich id auf null und leeren String prüfen, statt asBoolean aufzurufen, Die Exception würde ich dann auch von der statischen Methode werfen.

done

remoting/dolphin-remoting-server/src/main/groovy/org/opendolphin/core/server/action/ClosureServerAction.java, line 15 at r1 (raw file):

Previously, blackdrag (Jochen Theodorou) wrote…

Damit wirst du natürlich noch nicht die Groovy-Runtime los

Korrekt. Sowas will ich ja dann später mit dir diskutieren. Aber so ist es schon mal Java. Ist ja ok, wenn wir im Step 1 alles als Java Klassen haben die Intern aber noch die Groovy Runtime verwenden. Dann können wir das schon nach src/main/java verschieben und dann die Groovy Runtime als nächsten Schritt ausbauen

Comments from Reviewable

hendrikebbers added some commits Nov 16, 2016
@hendrikebbers hendrikebbers Based on review
d36573d
@hendrikebbers hendrikebbers Based on review
026bf27
@coveralls

Coverage Status

Coverage increased (+0.5%) to 69.13% when pulling 026bf27 on next-groovy-to-java into 26c9664 on master.

@coveralls

Coverage Status

Coverage increased (+0.5%) to 69.13% when pulling 026bf27 on next-groovy-to-java into 26c9664 on master.

@hendrikebbers hendrikebbers merged commit e6bbb4a into master Nov 16, 2016

3 of 4 checks passed

code-review/reviewable 2 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 remained the same at 69.13%
Details
@hendrikebbers hendrikebbers deleted the next-groovy-to-java branch Nov 16, 2016
@aalmiray aalmiray removed the in progress label Nov 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment