Rename `Telegram.group_address` to `Telegram.destination_address`. #510

basilfx · 2020-11-25T23:41:45Z

Description

This PR is a part of #253, for easier reviewing.

In this PR, I refactored Telegram.group_address to ~~Telegram.address~~ telegram.destination_address. Based on the type of address, the CEMIFrame is build with the proper flags set. This changes allows for the other changes of #253 to support non-broadcast APCI services.

All high-level devices still use group_address, which still makes sense over there. They just update a Telegram.destination_address with their group_address.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

I have performed a self-review of my own code
The documentation has been adjusted accordingly
The changes generate no new warnings
Tests have been added that prove the fix is effective or that the feature works
The changes are documented in the changelog
The Homeassistant plugin has been adjusted in case of new config options

marvin-w

I like this change. Thank you! Splitting it apart makes a lot of sense considering the size of the original PR. Once the minor issue is fixed we can merge this.

xknx/telegram/telegram.py

marvin-w

Looks nice, thank you!

Hint: Please don't force push once someone reviewed it as it's hard to find the actual changes.

basilfx · 2020-11-25T23:57:48Z

I'm aware of the hint, but in this case I did it directly since the change was so small ;-)

Next time I'll do a fixup commit and then squash once approved.

marvin-w · 2020-11-25T23:59:39Z

Cool! 👍

And yes, it was really small 👯

No need to manually squash. We can just squash the whole PR when merging.

coveralls · 2020-11-26T04:13:00Z

Coverage increased (+0.005%) to 94.499% when pulling 7ad8f3d on basilfx:feature/telegram_address into ffec8c1 on XKNX:master.

farmio · 2020-11-26T06:08:15Z

Since a Telegram has a destination address and a source address it would be nice to call it eg. one of "destination_address", "dest_address", "destination".
I know our Telegram object currently doesn't contain the source address, but I'd like to add it for eg. #497 and I can imagine other useful cases for source address in Device class.

basilfx · 2020-11-26T07:00:18Z

@farmio Yes, that would be awesome. But I would prefer to do it in another PR. It's quite some work to adapt it, and it doesn't 'help' me to achieve the goal of #253.

farmio · 2020-11-26T07:25:10Z

Just to get this straight: I just want to avoid renaming Telegram.group_address twice.
Adding source to Telegram should definitely be done in another PR (whenever and by whoever likes to)

basilfx · 2020-11-26T07:29:23Z

That's fine. So let's call it destination_address address then? That should be OK to adapt :)

farmio · 2020-11-26T08:20:33Z

Great! Descriptive is always nice. 😃

This renames `Telegram.group_address` to `Telegram.address`. This makes it possible to use a telegram for non-broadcast types of messages.

Per feedback, rename this to `Telegram.destination_address`.

basilfx · 2020-11-26T19:50:19Z

Rebased the PR and adapted Telegram.address to Telegram.destination_address.

marvin-w · 2020-11-28T17:30:10Z

@basilfx Please fix the merge conflicts and the linting error (black formatting).

https://travis-ci.org/github/XKNX/xknx/jobs/746142703#L349

Pre-commit will do this automatically for you.

EDIT: Took care of it already.

# Conflicts: # changelog.md

basilfx · 2020-11-28T19:53:07Z

Pushed one more to changelog.md since the field was renamed.

marvin-w · 2020-11-28T20:16:09Z

Thanks! Merged.

farmio · 2020-11-28T21:20:08Z

xknx/remote_value/remote_value.py

@@ -110,13 +110,13 @@ def to_knx(self, value):

    async def process(self, telegram, always_callback=False):
        """Process incoming or outgoing telegram."""
-        if not self.has_group_address(telegram.group_address):
+        if not self.has_group_address(telegram.destination_address):


will this raise TypeError if destination_address is a PhysicalAddress ? Could this even happen? Is this tested?

xknx/xknx/telegram/address.py

Line 64 in f1f654f

if type(self) is not type(other):

basilfx mentioned this pull request Nov 25, 2020

[WIP] Add support for other APCI services #253

Closed

marvin-w requested changes Nov 25, 2020

View reviewed changes

xknx/telegram/telegram.py Outdated Show resolved Hide resolved

basilfx force-pushed the feature/telegram_address branch from f99fa91 to 767540e Compare November 25, 2020 23:52

marvin-w approved these changes Nov 25, 2020

View reviewed changes

basilfx added 6 commits November 26, 2020 20:48

Rename Telegram.group_address

4e0ada4

This renames `Telegram.group_address` to `Telegram.address`. This makes it possible to use a telegram for non-broadcast types of messages.

Set CEMI frame flags based on address type

8eca427

Add tests for address CEMI frame flags

6c2a41a

Update changelog.md

81115ae

Add CEMI frame test for coverage

2a8408b

Rename Telegram.address

6028759

Per feedback, rename this to `Telegram.destination_address`.

basilfx force-pushed the feature/telegram_address branch from 767540e to 6028759 Compare November 26, 2020 19:49

basilfx changed the title ~~Rename Telegram.group_address to Telegram.address.~~ Rename Telegram.group_address to Telegram.destination_address. Nov 26, 2020

Marvin Wichmann and others added 3 commits November 28, 2020 20:30

Merge branch 'master' into feature/telegram_address

5e507a0

# Conflicts: # changelog.md

Review: Black formatting

0b32417

Update changelog.md

7ad8f3d

marvin-w merged commit f1f654f into XKNX:master Nov 28, 2020

farmio reviewed Nov 28, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename `Telegram.group_address` to `Telegram.destination_address`. #510

Rename `Telegram.group_address` to `Telegram.destination_address`. #510

basilfx commented Nov 25, 2020 •

edited

Loading

marvin-w left a comment

marvin-w left a comment

basilfx commented Nov 25, 2020

marvin-w commented Nov 25, 2020

coveralls commented Nov 26, 2020 •

edited

Loading

farmio commented Nov 26, 2020 •

edited

Loading

basilfx commented Nov 26, 2020

farmio commented Nov 26, 2020

basilfx commented Nov 26, 2020

farmio commented Nov 26, 2020

basilfx commented Nov 26, 2020

marvin-w commented Nov 28, 2020 •

edited

Loading

basilfx commented Nov 28, 2020

marvin-w commented Nov 28, 2020

farmio Nov 28, 2020 •

edited

Loading

Rename Telegram.group_address to Telegram.destination_address. #510

Rename Telegram.group_address to Telegram.destination_address. #510

Conversation

basilfx commented Nov 25, 2020 • edited Loading

Description

Type of change

Checklist:

marvin-w left a comment

Choose a reason for hiding this comment

marvin-w left a comment

Choose a reason for hiding this comment

basilfx commented Nov 25, 2020

marvin-w commented Nov 25, 2020

coveralls commented Nov 26, 2020 • edited Loading

farmio commented Nov 26, 2020 • edited Loading

basilfx commented Nov 26, 2020

farmio commented Nov 26, 2020

basilfx commented Nov 26, 2020

farmio commented Nov 26, 2020

basilfx commented Nov 26, 2020

marvin-w commented Nov 28, 2020 • edited Loading

basilfx commented Nov 28, 2020

marvin-w commented Nov 28, 2020

farmio Nov 28, 2020 • edited Loading

Choose a reason for hiding this comment

Rename `Telegram.group_address` to `Telegram.destination_address`. #510

Rename `Telegram.group_address` to `Telegram.destination_address`. #510

basilfx commented Nov 25, 2020 •

edited

Loading

coveralls commented Nov 26, 2020 •

edited

Loading

farmio commented Nov 26, 2020 •

edited

Loading

marvin-w commented Nov 28, 2020 •

edited

Loading

farmio Nov 28, 2020 •

edited

Loading