Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add isTransform(), isEmpty() and isPresent() to ChannelTransformation #4355

Merged
merged 3 commits into from
Aug 30, 2024

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Aug 19, 2024

@jimtng jimtng requested a review from a team as a code owner August 19, 2024 03:20
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng jimtng changed the title Add isEmpty() and isPresent() to ChannelTransformation Add isTransform(), isEmpty() and isPresent() to ChannelTransformation Aug 21, 2024
@jimtng
Copy link
Contributor Author

jimtng commented Aug 21, 2024

Modbus refactoring also requires this PR.

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/syntax-for-binding-transformations/157861/1

@J-N-K
Copy link
Member

J-N-K commented Aug 24, 2024

I don't understand the need for .isEmpty and .isPresent. In case it is empty, the original value is returned, so what will break if we call .apply in every case?

@jimtng
Copy link
Contributor Author

jimtng commented Aug 24, 2024

See https://github.com/openhab/openhab-addons/blob/0d6feb72aa23dcfce946e1367ea0ce8b8c03b330/bundles/org.openhab.binding.mqtt.generic/src/main/java/org/openhab/binding/mqtt/generic/ChannelState.java#L340

We need the ability to check if a transformation is at all defined. If it's defined, the command to be published will be converted to StringType, and subsequently formatBeforePublish will treat the data as a String data.

If it's not defined, then the original command type will be preserved (not converted to StringType) and the formatBeforePublish can apply type-specific formatting, e.g. special date format.

So that's why we need to be able to know whether transformation is defined.

I can instead check if config.transformationPatternOut is null or blank, and set the outgoingTransformation to null, and check it that way if you don't think we should add isEmpty/isPresent here.

However since ChannelTransformation already does a no-op (return input as output) when it has no transformations (i.e. empty), it's a lot more convenient to add isEmpty/isPresent to it than dealing with null checks around it.

Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

IMO it would be fine to do this where needed, but I'm ok with adding it here. Please see my comments.

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

LGTM

@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label Aug 30, 2024
@J-N-K J-N-K merged commit 8a59844 into openhab:main Aug 30, 2024
5 checks passed
@J-N-K
Copy link
Member

J-N-K commented Aug 30, 2024

I have triggered a new core snapshot, should be available in about 35 minutes.

@jimtng jimtng deleted the channeltransformation-isempty branch August 30, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants