Implemented configuration status handling for hue bridge.

[alex-kostadinov]

Implemented config status handling for hue bridge (unit tests included).

Signed-off-by: Alexander Kostadinov alexander.g.kostadinov@gmail.com

[maggu2810]

I have not reviewed this PR, just a short look at the changes.
At least the file "HueConfigStatusMessage.java" does not seem to be have correct format (it includes tabs, etc.)

[maggu2810]

The message is taken from the XML, why do we need to add one here, too?

[alex-kostadinov]

@maggu2810 I updated the PR and applied the changes you requested (considering formatter and HueConfigStatusMessage.java). Successful build run and functionality re-tested.

[tomhoefer]

why dont we simply use:

Configuration configuration = new Configuration().with {
            put(HOST, null)
            put(SERIAL_NUMBER, "testSerialNumber")
            it
        }

        createBridgeThing(configuration)

        HueBridgeHandler hueBridgeHandler = getRegisteredHueBridgeHandler()

        def expected = ConfigStatusMessage.Builder.error(HOST).withMessageKey(HueConfigStatusMessage.IP_ADDRESS_MISSING
                .getMessageKey()).build()

        waitForAssert {
            assertThat hueBridgeHandler.getConfigStatus().first(), is(expected)
        }

[alex-kostadinov]

@tomhoefer The PR is updated and changes you required for the HueBridgeHandler, as well as for the HueBridgeHandlerOSGiTest, are applied. Successful build run and functionality re-tested.

[tomhoefer]

I would appreciate if we could avoid the usage of this collection and the builder instance below...

if necessary we should directly return in the corresponding sections with ImmtuableList.of(ConfigStatusMessage.Builder.error(...).xxx)

[alex-kostadinov]

@tomhoefer PR updated according to latest comments and travis-ci build successfully passed.

[tomhoefer]

@kaikreuzer lgtm, if you dont have any further comments we can merge this PR

[kaikreuzer]

Why is it necessary to define the uri here? Isn't that identical to the uri that is used as a default? Unless there is a good reason to overwrite the default, I would not recommend doing this.

[tomhoefer]

yes, you are right... the URI can be removed

[tomhoefer]

@AlexanderKostadinov28 ConfigStatusBridgeHandler is now available #1898

[tomhoefer]

@AlexanderKostadinov28 message key handling has also been improved by #1933

[kaikreuzer]

Any update here?

[alex-kostadinov]

Resolved conflicting file.

[kaikreuzer]

License header should be updated, the rest is very minor, which could be done along the way.

[kaikreuzer]

could you please update the license header to the current one?

[alex-kostadinov]

I will replace the header with the currently used one.

[kaikreuzer]

When you touch this file anyhow, could you please remove this empty line?

[alex-kostadinov]

Yes, I will remove the blank line before the return statement.

[kaikreuzer]

better add an "else" here.

[alex-kostadinov]

Fine, an else statement will be added. What is your opinion about the approach of first declaring the collection, then assigning it inside the if/else statement and finally using a single return?

Collection<ConfigStatusMessage> configstatusMessages;

// Check whether an IP address is provided
if (bridgeIpAddress == null) {
    configstatusMessages = Collections.singletonList(ConfigStatusMessage.Builder.error(HOST)
        .withMessageKeySuffix(HueConfigStatusMessage.IP_ADDRESS_MISSING.getMessageKey())
        .withArguments(HOST).build());
} else {
     configstatusMessages = Collections.emptyList();
}

return configstatusMessages;

Furthermore, what do you think about inverting the logic and checking if the bridge IP address is not null and also matches an IPv4 matcher, such as bridgeIpAddress.matches("^(?:[0-9]{1,3}\\.){3}[0-9]{1,3}$")?

All in all, please share your opinion about making the scope of the check greater (IP not null and can be successfully filtered through the matcher) instead of just checking for null value.

Collection<ConfigStatusMessage> configstatusMessages;

// Check whether an IP address is provided
if (bridgeIpAddress != null && bridgeIpAddress.matches("^(?:[0-9]{1,3}\\.){3}[0-9]{1,3}$")) {
    configstatusMessages = Collections.emptyList();
} else {
    configstatusMessages = Collections.singletonList(ConfigStatusMessage.Builder.error(HOST)
        .withMessageKeySuffix(HueConfigStatusMessage.IP_ADDRESS_MISSING
        .getMessageKey()).withArguments(HOST).build());
}

return configstatusMessages;

I will of course move the matcher in a class constant.

[tomhoefer]

I would not use a regex expression to check for a valid IPv4 address... The IP address is something that can simply be checked by the ConfigDescriptionValidator. For this reason we have to modify the contexts of the config description parameters. Currently there is only the context network-address which is not specific enough to validate it... @kaikreuzer what is your opinion regarding this?

Adding the additional isEmpty check is ok for me...

btw, I would not add the additional else statement, but this is a matter of taste

[kaikreuzer]

@kaikreuzer what is your opinion regarding this?

I fully agree. The network address could be IPv6 or a hostname as well - both is valid.

[kaikreuzer]

What is your opinion about the approach of first declaring the collection, then assigning it inside the if/else statement and finally using a single return?

This does not really matter much to me - both approaches are fine.

[kaikreuzer]

Is this any better than the normal Collections.singletonList(o)? If not necessary, I would not introduce the use of Guava in this class.

[alex-kostadinov]

To me it makes sense, since Collections.singletonList(o) is already in use for the class and there is no need to make use of com.google.common.collect.ImmutableList. @tomhoefer, what is your opinion on the subject, since we previously had a discussion and you insisted that I introduce the use of Guava and ImmutableList.of()?

[tomhoefer]

ok for me... with the suggestion of using ImmutableList.of I wanted to make you only aware of using an unmodifiable collection....

[alex-kostadinov]

Implemented requested changes (updated header and did the other mentioned minor changes), as well as implemented an additional unit test for the bridge test suite so that both cases (IP is null or empty) are covered.

[kaikreuzer]

Thanks, lgtm now!