Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Fix thing creation for nested Things from different ThingHandlerFactory #5410

Merged
merged 5 commits into from
Apr 23, 2018

Conversation

htreu
Copy link
Contributor

@htreu htreu commented Apr 12, 2018

Fixes #5404.

The first commit is only a reorganisation of the tests into sane packages.

Copy link
Contributor

@sjsf sjsf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to minor remarks, but it looks correct to me.

À propos: I vaguely remember us talking about a test-case which you wrote which proved the broken behavior - or do I mix something up? Did you maybe forget to commit it, or did I miss it anywhere?

// construct the thingUID up front: this will ensure that short notation things with reference to a bridge
// do not end up with the bridge id in their id.
things.forEach([thingId = thingId ?: constructThingUID.toString])
things.filter(typeof(ModelBridge)).forEach([val b = it; b.things.forEach[
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val b = it; -> b |

Nevermind, all correct. Still, here you got me - how about giving both its some explicit names?

// for nested things in short notation, make sure the bridge id is part of the thing id.
id = id ?: getThingUID(new ThingUID(b.id)).toString;
]])
things + flattenModelThings(things.filter(typeof(ModelBridge)).map(b | b.things).flatten);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just personal preference, but here I think a return would help readability


String model =
'''
Bridge Xhue:Xbridge:myBridge [ XipAddress = "1.2.3.4", XuserName = "123" ] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaaaaahhh, TABS!!! 😱

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto-formatter refuses to make all good, sorry :-(

@htreu
Copy link
Contributor Author

htreu commented Apr 19, 2018

@SJKA thanks for reminding me about the test case. I wrote one and apparently the fix is still incomplete. In case a bundle defines multiple HandlerFactories (which should be possible imho) it fails. And thus would fail for the reason this fix was made.
stay tuned.

* introduce BundleNameResolver

Signed-off-by: Henning Treu <[email protected]>
@htreu htreu force-pushed the 5404_fix_nested_things branch from 03453aa to 773fe06 Compare April 19, 2018 14:50
@@ -0,0 +1,6 @@
package org.eclipse.smarthome.model.thing.internal.util;

public interface BundleNameResolver {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need this additional interface? Wouldn't a making the getBundleName(...) method protected be sufficient, so that it can be overridden in the test-case by like this?

new BundleNameResolver() {
    @Override
    protected String getBundleName(...) {
        // ...
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. I didn't like the solution either.

@kaikreuzer kaikreuzer merged commit 7dcc8b4 into eclipse-archived:master Apr 23, 2018
@htreu htreu deleted the 5404_fix_nested_things branch April 24, 2018 07:50
ermartens pushed a commit to ermartens/smarthome that referenced this pull request Jun 15, 2018
…ry (eclipse-archived#5410)

* Refactor source directories and packages
* flatten bridge models before creating things
* Flatten thing models before handler creation
* add test for multiple ThingHandlerFactories
* introduce BundleNameResolver

Signed-off-by: Henning Treu <[email protected]>
@htreu htreu added this to the 0.10.0 milestone Oct 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants