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

[OWM] Store / cache icon files in a local folder and minor code improvements #6533

Merged
merged 1 commit into from Dec 3, 2018
Merged

[OWM] Store / cache icon files in a local folder and minor code improvements #6533

merged 1 commit into from Dec 3, 2018

Conversation

cweitkamp
Copy link
Contributor

@cweitkamp cweitkamp commented Nov 18, 2018

  • Store / cache image files in a local folder
  • Reduced discovery timeout
  • Minor code improvements

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

@cweitkamp cweitkamp changed the title [WIP][OWM] Added new channels for sunrise / sunset, store / cache icon files in a local folder [OWM] Added new channels for sunrise / sunset, store / cache icon files in a local folder Nov 20, 2018
@kaikreuzer
Copy link
Contributor

Hm, I assume the sunrise/sunset information is no "sensor" weather data at all, but purely calculated by OWM, right? If so, it completely duplicates the functionality of the Astro binding, which already provides all this (and much more). I would thus suggest to not duplicate such a feature here (especially with the all the configuration like offset/earliest/latest, which clearly targets shutter automation).
Or is there anything special about OWM why the values provided are better than the ones calculated by the Astro binding?

@cweitkamp
Copy link
Contributor Author

Or is there anything special about OWM why the values provided are better than the ones calculated by the Astro binding?

No, I do not think so. The OWM documentation does not tell anything about that. I assume the values are calculated. It is true that I copied a lot of functionality from the Astro binding. Adding the event handling here provides no general advantages. I am personally interested only in the sunrise/sunset triggers of the Astro binding thus - for my own sake - I can avoid using the Astro binding and would not loose any of the features.

@kaikreuzer
Copy link
Contributor

for my own sake - I can avoid using the Astro binding

That's a very weak argument for duplicating a lot of code - I'd definitely vote for removing it from this binding.

@cweitkamp
Copy link
Contributor Author

Yes, I agree. I will remove it.

@cweitkamp
Copy link
Contributor Author

I removed the event handling.

@kaikreuzer
Copy link
Contributor

Hm, I was actually talking about the "the sunrise/sunset information", i.e. the channels themselves - there shouldn't be any good reason to use them instead of the Astro binding...

- Removed UTF-8 constant
- Store / cache image files in a local folder
- Reduced discovery timeout
- Added log output in case the daily forecast is not available

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@cweitkamp cweitkamp changed the title [OWM] Added new channels for sunrise / sunset, store / cache icon files in a local folder [OWM] Store / cache icon files in a local folder and minor code improvements Dec 3, 2018
@cweitkamp
Copy link
Contributor Author

Alright, I removed them completely.

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.

Thanks!

@kaikreuzer kaikreuzer merged commit 385b756 into eclipse-archived:master Dec 3, 2018
@cweitkamp cweitkamp deleted the feature-openweathermap-binding-fixes branch December 3, 2018 15:26
@kaikreuzer kaikreuzer added this to the 0.10.0 milestone Dec 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants