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

[http] Initial contribution #8521

Merged
merged 3 commits into from
Nov 9, 2020
Merged

[http] Initial contribution #8521

merged 3 commits into from
Nov 9, 2020

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Sep 21, 2020

This adds a new HTTP binding as a replacement for the old http1 binding.

Replaces #7851

Signed-off-by: Jan N. Klug jan.n.klug@rub.de

@J-N-K J-N-K requested a review from a team as a code owner September 21, 2020 16:40
@kaikreuzer kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Sep 21, 2020
@TravisBuddy
Copy link

TravisBuddy commented Sep 21, 2020

Hey @J-N-K,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 95074880-04e0-11eb-a4bc-1b94c014fd22

@kaikreuzer
Copy link
Member

PR build asks you to apply spotless to the pom.xml.

@Hilbrand Hilbrand added new binding If someone has started to work on a binding. For a new binding PR. oh1 migration Relates to migrating an openHAB 1 addon to openHAB 2 labels Sep 22, 2020
@cpmeister cpmeister added the cre Coordinated Review Effort label Oct 2, 2020
@J-N-K J-N-K added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Oct 31, 2020
@J-N-K J-N-K force-pushed the http branch 2 times, most recently from fe05dc6 to 0e23276 Compare October 31, 2020 16:45
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
@J-N-K
Copy link
Member Author

J-N-K commented Nov 8, 2020

@openhab/add-ons-maintainers Can we get a second review here? Would be great to have this in M3, so people using http1 can move on to OH3

@fwolter fwolter self-assigned this Nov 8, 2020
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

Good code in general. I'm not aware of the current null check implementations, but I receive 3 warnings when compiling with maven.

I don't really like that you use unchecked exceptions all over the code, because the compiler doesn't enforce you to catch those. Normally I would claim this as error prone, but I think you know what you're doing.

bundles/org.openhab.binding.http/.classpath Outdated Show resolved Hide resolved
bundles/org.openhab.binding.http/.project Outdated Show resolved Hide resolved

| parameter | optional | default | description |
|-------------------------|----------|-------------|-------------|
| `onValue` | yes | - | A special value that represents `ON` |
Copy link
Member

Choose a reason for hiding this comment

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

By reading the documentation I don't understand what these special values are about. Are they searched in the response body respectively sent in the body?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the response (after transformation) is equal to that value, it is interpreted as ON (e.g. a response could be true, 1 or eingeschalltet).

Copy link
Member

Choose a reason for hiding this comment

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

Can you put this sentence in the readme? I think it would help the user to understand how this binding is actually working, as you don't provide examples.

When thinking about how this works, I also wouldn't know what to set the transformation parameters to. I think a full example would be really helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a note for the special values and also examples for the transformation

bundles/org.openhab.binding.http/README.md Outdated Show resolved Hide resolved
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
@J-N-K
Copy link
Member Author

J-N-K commented Nov 8, 2020

Regarding the checked/unchecked exceptions: Unfortunately an unchecked exception leads to a better code style.

refreshingUrlCache.get().ifPresent(itemValueConverter::process);

is not possible if ItemValueConverter.process(Content content) throws a check exception. The exception handling would need to be done insinde of ifPresent.


| parameter | optional | default | description |
|-------------------------|----------|-------------|-------------|
| `onValue` | yes | - | A special value that represents `ON` |
Copy link
Member

Choose a reason for hiding this comment

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

Can you put this sentence in the readme? I think it would help the user to understand how this binding is actually working, as you don't provide examples.

When thinking about how this works, I also wouldn't know what to set the transformation parameters to. I think a full example would be really helpful.

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

LGTM

@fwolter fwolter merged commit 8de5652 into openhab:main Nov 9, 2020
@fwolter fwolter added this to the 3.0.0.M3 milestone Nov 9, 2020
@J-N-K J-N-K deleted the http branch November 9, 2020 17:16
@J-N-K J-N-K mentioned this pull request Dec 6, 2020
nowaterman pushed a commit to nowaterman/openhab-addons that referenced this pull request Jan 19, 2021
boehan pushed a commit to boehan/openhab-addons that referenced this pull request Apr 12, 2021
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cre Coordinated Review Effort new binding If someone has started to work on a binding. For a new binding PR. oh1 migration Relates to migrating an openHAB 1 addon to openHAB 2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants