Qualifier Support for DP #205

Merged
merged 4 commits into from Aug 15, 2016

Projects

None yet

3 participants

@hendrikebbers
Member
hendrikebbers commented Jul 7, 2016 edited

This PR add qualifier support for the Dolphin Platform.

To use qualifiers a QualifierBinder can be injected (see Test example).
The qualifiers of the DP are using generics for a better type safety.


This change is Reviewable

@hendrikebbers hendrikebbers Qualifier Support for DP
31fa9f7
@hendrikebbers hendrikebbers added this to the 0.8.7 milestone Jul 7, 2016
@aalmiray aalmiray added the in progress label Jul 7, 2016
@hendrikebbers
Member

I ask myself if we should rename QualifierBinder to PropertyBinder ;)

@coveralls

Coverage Status

Coverage decreased (-0.07%) to 69.605% when pulling 31fa9f7 on qualifier into 57c722c on master.

@hendrikebbers
Member

BindingException is too plain. it could be richer by receiving the property name, identifier and other arguments; crafting the message internally and providing access to some of its args if saved on final fields. IOW BindingException is too close to plain RuntimeException.

@hendrikebbers hendrikebbers QualifierBinder -> PropertyBinder
da0ecd8
@coveralls

Coverage Status

Coverage decreased (-0.07%) to 69.605% when pulling da0ecd8 on qualifier into 57c722c on master.

@hendrikebbers hendrikebbers Javadoc
b8fe2b4
@coveralls

Coverage Status

Coverage decreased (-0.2%) to 69.524% when pulling b8fe2b4 on qualifier into 57c722c on master.

@aalmiray
Contributor

Reviewed 23 of 32 files at r1, 6 of 8 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/ReflectionHelper.java, line 97 [r3] (raw file):

        Assert.requireNonNull(type, "type");
        Assert.requireNonNull(name, "name");
        for(Field field : getInheritedDeclaredFields(type)) {

the side effect of this call is to compute the full list of fields and then filter it. Given that we need to find a single one I'd say the behavior of getInheritedDeclaredFields(type) should be inlined here, that is, search and filter as you go.


platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/binding/Qualifier.java, line 13 [r3] (raw file):

 * @see PropertyBinder
 */
public class Qualifier<T> {

should this type be subclassed? if not make it final.


Comments from Reviewable

@hendrikebbers
Member

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/ReflectionHelper.java, line 97 [r3] (raw file):

Previously, aalmiray (Andres Almiray) wrote…

the side effect of this call is to compute the full list of fields and then filter it. Given that we need to find a single one I'd say the behavior of getInheritedDeclaredFields(type) should be inlined here, that is, search and filter as you go.

Make sense. Can you fix it? By doing so you can merge the PR.

platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/binding/Qualifier.java, line 13 [r3] (raw file):

Previously, aalmiray (Andres Almiray) wrote…

should this type be subclassed? if not make it final.

Make sense. Can you fix it? By doing so you can merge the PR.

Comments from Reviewable

@hendrikebbers
Member

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/ReflectionHelper.java, line 97 [r3] (raw file):

Previously, hendrikebbers (Hendrik Ebbers) wrote…

Make sense. Can you fix it? By doing so you can merge the PR.

done

platform/dolphin-platform-server/src/main/java/com/canoo/dolphin/server/binding/Qualifier.java, line 13 [r3] (raw file):

Previously, hendrikebbers (Hendrik Ebbers) wrote…

Make sense. Can you fix it? By doing so you can merge the PR.

done

Comments from Reviewable

@hendrikebbers hendrikebbers Merge branch 'master' into qualifier
# Conflicts:
#	platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/ReflectionHelper.java
cee1c5a
@hendrikebbers hendrikebbers merged commit 79a5095 into master Aug 15, 2016

2 of 3 checks passed

code-review/reviewable 2 files, 2 discussions left (aalmiray)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@aalmiray aalmiray removed the in progress label Aug 15, 2016
@hendrikebbers hendrikebbers deleted the qualifier branch Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment