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

[openweathermap] Add support for One Call API #7308 #8395

Closed
wants to merge 25 commits into from
Closed

[openweathermap] Add support for One Call API #7308 #8395

wants to merge 25 commits into from

Conversation

Wolfgang1966
Copy link

This PR adds support for the One Call API of Openweathermap as requested in #7308 . I tried to follow the structure of the original binding and touch it as little as possible. Especially, all existing functionality will continue to work, there are no breaking changes. The channel definitions follow the naming conventions of the original binding as far as possible, so it should be easy to adapt existing config files to the One Call API version for all channels that are supported by both APIs.

JARs for testing have already been provided and may have already been tested; I did not yet receive any feedback on it. Find them here, together with example definition files:

https://home.klimt.de/owncloud/index.php/s/BgnsYtxAAYFcZ7f

Before I started I was already in Contact with @cweitkamp , the original developer of the binding, and followed his advice on how to configure the gson mapping used by the binding.

Sign-Off has been added to all my commits.

It would be great if the PR would make it into the 2.5.9 release.

Thank you very much

Wolfgang

kaikreuzer and others added 4 commits September 4, 2020 10:20
Signed-off-by: Kai Kreuzer <kai@openhab.org>
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: wolfii <wolfgang@klimt.de>
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: wolfii <wolfgang@klimt.de>
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: wolfii <wolfgang@klimt.de>
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
@TravisBuddy
Copy link

Travis tests were successful

Hey @Wolfgang1966,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@Hilbrand Hilbrand added the enhancement An enhancement or new feature for an existing add-on label Sep 4, 2020
@Hilbrand Hilbrand changed the title #7308 [openweathermap] Add support for One Call API [openweathermap] Add support for One Call API #7308 Sep 4, 2020
@Hilbrand
Copy link
Member

Hilbrand commented Sep 4, 2020

Did you see at #8253?

@Wolfgang1966
Copy link
Author

Did you see at #8253?

Yes, I also had contact with @MPH80 recently, see #7308 (comment)

His pull request changed the existing implementation and lost backward compability, as the one call API does not cover all functionality the existing binding offers. This is why I decided to add new things and leave the existing ones untouched. I also had a look at his PR to check whether I could modify it, but finally found it easier to base my changes on the original binding.

@MPH80
Copy link

MPH80 commented Sep 4, 2020

Did you see at #8253?

Yes, I also had contact with @MPH80 recently, see #7308 (comment)

His pull request changed the existing implementation and lost backward compability, as the one call API does not cover all functionality the existing binding offers. This is why I decided to add new things and leave the existing ones untouched. I also had a look at his PR to check whether I could modify it, but finally found it easier to base my changes on the original binding.

Indeed - I need to close my existing PR. Let me do that now.

wolfii added 2 commits September 4, 2020 11:58
Corrected README.md
Corrected reference in thing-types.xml

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
@TravisBuddy
Copy link

Travis tests have failed

Hey @Wolfgang1966,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

Code looks great. Besides the inline comments, some generic:

  • Can you remove the first commit? There are some changes in this pr that should not be here.
  • Can run mvn spotless:apply to fix the codestyle of the files.
  • There are a lot of channel(groups). I think some of those channels and/or channel groups can get attribute advanced.
  • Another option could be to dynamically create the groups. Either by making it extendable to let the user create them or by let the user configure the amount of history and create that amount of groups.

wolfii added 4 commits September 5, 2020 09:50
Signed-off-by: wolfii <wolfgang@klimt.de>
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: wolfii <wolfgang@klimt.de>
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: wolfii <wolfgang@klimt.de>
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Corrected README.md
Corrected reference in thing-types.xml

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
@Wolfgang1966
Copy link
Author

Dear all, especially @cweitkamp and @Hilbrand

As 2.5.9 is released now, I assume it makes no sense to get this PR merged to the 2.5.x tree. So I will have to provide a PR against the main branch for a 3.0 version of this binding, correct?

I try to get the PR ready asap, but please note that ASAP at least means in a couple of days from now.

I will continue to provide snapshot builds of the 2.5.X version of this binding version at https://home.klimt.de/owncloud/index.php/s/BgnsYtxAAYFcZ7f for all who need a 2.5 version.

Should I then close this PR or is there any chance that it might still be useful?

Yours

Wolfgang

@boehan
Copy link
Contributor

boehan commented Sep 27, 2020

@Wolfgang1966 just a little remark: Since I just looked at your latest changes, could it be that you missed some of review comments that are hidden in the comment history (under "16 hidden conversations. Load more...")?

@Wolfgang1966
Copy link
Author

@Wolfgang1966 just a little remark: Since I just looked at your latest changes, could it be that you missed some of review comments that are hidden in the comment history (under "16 hidden conversations. Load more...")?

Oh F****, you are right! :-(

Will work on them ASAP (in the next days!). Feeling stupid now ... 👎

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
@Wolfgang1966
Copy link
Author

Dear all, I worked on the comments from @cweitkamp I overlooked last time. Pushed all the changes now.

A new version of the binding is now available at
https://home.klimt.de/owncloud/index.php/s/Y7P4mWfXJs7iWNW
(the old link is no longer valid)

Next step will be to create a OH 3.0 version of the binding and a PR for that.

@Wolfgang1966
Copy link
Author

I will continue to provide snapshot builds of the 2.5.X version of this binding version at https://home.klimt.de/owncloud/index.php/s/BgnsYtxAAYFcZ7f for all who need a 2.5 version.

Use
https://home.klimt.de/owncloud/index.php/s/Y7P4mWfXJs7iWNW
instead

@boehan
Copy link
Contributor

boehan commented Oct 21, 2020

Did some initial testing on the One Call thing and noticed some issues with the thing configuration or dynamic channels, respectively:

  • set "Number of days" to 3 --> channel groups for today, tomorrow and apparent tomorrow are shown (as expected) but also for day 6 and day 7.
  • if I set "Number of hours" to 0 --> a channel group for hourly forecast (hour01) is still shown.
  • if I set "Number of minutes" to something <3 --> channel groups for Minutes01, Minutes02 and Minutes03 are still shown.

@acidburn78
Copy link

Where can I find the Oh3 version?

@Wolfgang1966
Copy link
Author

Sorry, still in progress. Too many other things to do :-(

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
@Wolfgang1966
Copy link
Author

Did some initial testing on the One Call thing and noticed some issues with the thing configuration or dynamic channels, respectively:

  • set "Number of days" to 3 --> channel groups for today, tomorrow and apparent tomorrow are shown (as expected) but also for day 6 and day 7.
  • if I set "Number of hours" to 0 --> a channel group for hourly forecast (hour01) is still shown.
  • if I set "Number of minutes" to something <3 --> channel groups for Minutes01, Minutes02 and Minutes03 are still shown.

Dear @boehan , thanks for testing! I think I found the problem and corrected it. New version of the lib is available at

https://home.klimt.de/owncloud/index.php/s/Y7P4mWfXJs7iWNW

@Wolfgang1966
Copy link
Author

As 2.5.9 obviously was NOT the final 2.x release, I concentrate now on getting the binding approved and merged before working on the OH 3 version. So may I kindly ask for reviews (especially @cweitkamp )?

Thanks a lot

Wolfgang

@cweitkamp
Copy link
Contributor

I am afraid there will be no chance to get this into OH2.5.x anymore. Could you please re-create this PR against the main branch?
See point (4) in #8512. Thanks!

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
@Wolfgang1966
Copy link
Author

I finally managed to create a PR for OH3: #9062

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
@Wolfgang1966
Copy link
Author

Where can I find the Oh3 version?

OH3 version is now here: #9062

@cweitkamp
Copy link
Contributor

Superseded by #9062. Closing this now.

@cweitkamp cweitkamp closed this Nov 22, 2020
@Wolfgang1966 Wolfgang1966 deleted the 2.5.x branch May 2, 2022 14:09
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 for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants