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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrations rework & MeteoFrance integration #18

Merged
merged 8 commits into from
May 22, 2021

Conversation

Nsbx
Copy link
Contributor

@Nsbx Nsbx commented May 20, 2021

Hello 馃槃

I found your project and thought it was really cool. The only problem was with MeteoAlarm, i tried multiple time but impossible to make it works. I also tried with the custom integration MeteoAlarmeu same problem.
So i made some modification to make your card works with the official integration "MeteoFrance" (https://www.home-assistant.io/integrations/meteo_france/).

I made some refactoring to use the design pattern "strategy"

So the code can be easily extended to much other integration.

I also fixed typo and fix the code style (auto indent from vscode)

I'm French native, and i work mostly with PHP so i'm sorry for my bad english and probably my bad JS :')

If you have question or you want i do some modification don't hesitate.

Edit: I forget, i have transformed the code for meteoalarm & meteoalarmeu into "strategy" but because neither of them works on my instance i don't know if this work.

@Nsbx
Copy link
Contributor Author

Nsbx commented May 20, 2021

Hm i see the build error i fix that

@MrBartusek
Copy link
Owner

@Nsbx Hey, thanks for contributing! Build failed because you made a lot of code style changes that don't match eslint, I'm sure that most of them can be auto-fixed by it

@Nsbx Nsbx force-pushed the master branch 3 times, most recently from 01541d7 to c2c22b5 Compare May 21, 2021 10:18
@Nsbx
Copy link
Contributor Author

Nsbx commented May 21, 2021

Sorry for all code style change, i never use eslint before, i hope i fixed all my mistakes.
I also edited the readme to understand how the new "system" works.
I made this design pattern change because unfortunately the entity who "report" weather events is not binary sensor like meteoalarm.
But i thinks this is better like this, the whole system can be extended without touch to the core system

Copy link
Owner

@MrBartusek MrBartusek left a comment

Choose a reason for hiding this comment

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

Hey, I', glad that you had refactored that code! There are a couple of things that I don't like the thought

I don't think that user needs to select the type of their integration. There can easily be an automatic mode where strategy will be chosen based on the attributes of the sensor, supports() can be changed to do that for example. It still would be a good idea to give user ability to overwrite the automated system thought

I don't think that isAvaiable() should be written in every integration, i don't know any sensor that reports that they are unavailable other than setting their state to unavailable

I really don't like the Strategy name. It will be much better if these classes were to be named MeteoAlarmIntegration MeteoFranceIntegration etc.

I am not sure if that's needed to separate these integrations intro integrations and custom_integrations i bet there won't be more than 10 integrations anyway so it just complicates file structure.

@Nsbx
Copy link
Contributor Author

Nsbx commented May 21, 2021

Thank for your advices, i changed my code in consequence.
Also i discovered a problem, how do you manage multi-events ?
Currenlty in my city in France there is a wind and thunderstorm alert, but with my actual code only one of these event while be displayed.

@MrBartusek
Copy link
Owner

@Nsbx Well, the way that the card is made doesn't allow for displaying more than one event so it should display the one with higher severity and if there are a couple with the same severity, the more important one. For example, it's normal that if there is a thunderstorm there is also wind so a thunderstorm alert is more important.

@Nsbx
Copy link
Contributor Author

Nsbx commented May 21, 2021

@MrBartusek
I had thought about it but I wasn't sure, thank for your clarification.
I kept the same order of events of your class so i think this should works.
Edit: in any case this it is indeed the thunderstorm event displayed on my instance

@MrBartusek
Copy link
Owner

@Nsbx I've made some changes,

  • Meteoalarm and Meteoalarm weren't working, they are fixed now
  • Automatic now works a little bit differently, user can select integration type or do it automatically
  • Some general code clarity improvements

I think this PR is ready to be merged, just let me know are you OK with these changes

@Nsbx
Copy link
Contributor Author

Nsbx commented May 22, 2021

Thank you for your fix, i tried to not break the original code but without testing this is impossible.
If is ok for you let's go :D

@MrBartusek MrBartusek changed the title MeteoFrance integration Integrations rework & MeteoFrance integration May 22, 2021
@MrBartusek MrBartusek merged commit df7073e into MrBartusek:master May 22, 2021
BartEngelen pushed a commit to BartEngelen/MeteoalarmCard that referenced this pull request Jan 20, 2022
Co-authored-by: MrBartusek <bartusekcraft@gmail.com>
BartEngelen pushed a commit to BartEngelen/MeteoalarmCard that referenced this pull request Jan 21, 2022
Co-authored-by: MrBartusek <bartusekcraft@gmail.com>
BartEngelen pushed a commit to BartEngelen/MeteoalarmCard that referenced this pull request Jan 21, 2022
Co-authored-by: MrBartusek <bartusekcraft@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants