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

[core] Added implementation of a ChannelGroupUID #6103

Merged
merged 2 commits into from
Sep 4, 2018
Merged

[core] Added implementation of a ChannelGroupUID #6103

merged 2 commits into from
Sep 4, 2018

Conversation

cweitkamp
Copy link
Contributor

@cweitkamp cweitkamp commented Aug 23, 2018

Initial idea and for discussion.

As preparation for a ChannelGroupBuilder which is accessible from the ThingHandlerCallback similar to the createChannelBuilder() or editChannel() methods I would like to introduce a ChannelGroupUID to identify the channel group to be created / edited.

  • Added implementation and test of a ChannelGroupUID

Signed-off-by:Christoph Weitkamp github@christophweitkamp.de

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@htreu
Copy link
Contributor

htreu commented Aug 27, 2018

Hi @cweitkamp can you please add some more context on what plans you have for a ChannelGroupBuilder? Right now there is now separate entity ChannelGroup.

@cweitkamp
Copy link
Contributor Author

cweitkamp commented Aug 27, 2018

Yes, of course. Currently there is no easy way to create all channels for a specific ChannelGroupType. It has to be done manually by creating one channel after the other. I would like to introduce a new feature which can be used to create those channels all at once. An automatic approach needs access to the ChannelGroupTypeRegistry and/or the ChannelTypeRegistry which is not allowed for a binding.

I am not sure if we need a real builder for it or if we even need a ChannelGroup entity. We maybe can e.g. just return a list of ChannelBuilders.

public List<ChannelBuilder> createChannelGroupBuilder(ChannelGroupUID channelGroupUID, ChannelGroupTypeUID channelGroupTypeUID) {
    ChannelGroupType channelGroupType = channelGroupTypeRegistry.getChannelGroupType(channelGroupTypeUID);
    if (channelGroupType == null) {
        throw new IllegalArgumentException(String.format("Channel group type '%s' is not known", channelGroupTypeUID));
    }
    List<ChannelBuilder> channelBuilders = new ArrayList<>();
    for (ChannelDefinition channelDefinition : channelGroupType.getChannelDefinitions()) {
        ChannelType channelType = channelTypeRegistry.getChannelType(channelDefinition.getChannelTypeUID());
        if (channelType != null) {
            ChannelUID channelUID = new ChannelUID(channelGroupUID, channelDefinition.getId());
            channelBuilders.add(ThingFactoryHelper.createChannelBuilder(channelUID, channelType, configDescriptionRegistry));
        }
    }
    return channelBuilders;
}

@cweitkamp
Copy link
Contributor Author

@htreu Since I already posted an initial code snippet I have created some working code too.

https://github.com/cweitkamp/smarthome/compare/feature-channelgroupuid...cweitkamp:feature-channelgroupbuilder

@htreu
Copy link
Contributor

htreu commented Sep 4, 2018

Thanks for the example. This looks pretty good and straight forward.
Is there a real need for the builder right now? Or do you want to provide this feature from the framework so that future binding developments can use it?
Why I ask: imho we should only have code which is actively used. Maintaining it should be justified by actual usage.

@cweitkamp
Copy link
Contributor Author

I am with you. Unused code is just a millstone around the neck. But of course I have a use case for it.

The OWM binding (#5694) supports different forecast strategies (hourly / daily). Currently I decided to use a fixed number of hours (24) / days (5) to be requested whereas the OWM API is capable of delivering more data (max. 5 days / hour forecast and 16 days / daily forecast). Instead of defining fixed numbers I want to add configuration parameter for them. This should make it possible for everyone to adopt the number of forecasted hours / days for their own needs.

To archive my goal it would be nice to have an easy way to create a dynamical number of channel groups.

@htreu
Copy link
Contributor

htreu commented Sep 4, 2018

Yes, I just started to review the OWM binding and realised your needs in this. Thanks for the explanations.
I will merge here, so you can go on with the rest of the puzzle ;-)

@htreu htreu merged commit 06efd03 into eclipse-archived:master Sep 4, 2018
@cweitkamp cweitkamp deleted the feature-channelgroupuid branch September 4, 2018 08:53
Copy link
Contributor

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Although this is already merged, please allow me two comments that I think should be addressed.

public class ChannelGroupUID extends UID {

private static final String CHANNEL_GROUP_SEGMENT_PATTERN = "[\\w-]*#";
private static final String CHANNEL_GROUP_SEPERATOR = "#";
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: This should be SEPARATOR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe, that is a little copy&paste error from ChannelUID 😉. I will change it.

@Test
public void testChannelGroupUID() {
ChannelGroupUID channelGroupUID = new ChannelGroupUID(THING_UID, GROUP_ID);
assertEquals("binding:thing-type:thing:group#", channelGroupUID.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not in line with what the ChannelGroupTypeUID expects as a channelGroupUid (String) parameter - the separator should NOT be part of the UID (that anyhow looks a bit weird to me).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I prepare a follow up to remove the # at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #6157

@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