Skip to content

Conversation

@Delmael
Copy link
Collaborator

@Delmael Delmael commented Jan 14, 2025

Hi @ajtudela

Here the PR from your repo 😄

I've worked on this home assistant integration to mainly add the configuration by the UI.
I do not have a smartbox but a native HJM wifi heater which is working by the same plateform.

Changelog

  • Settings the integration throw UI

    • Adding main field for config entry (the username is the unique id)
    • Session options has an Options Flow
    • Reauthentication flow
    • Error handling
  • Improvement

If it fit your need I have other ideas 😃

Thanks,

Copy link
Owner

@ajtudela ajtudela left a comment

Choose a reason for hiding this comment

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

Hi, this is a very impressive PR. I also have an HJM emitter and it works to me.

I left minor comments that must be address.

Otherwise, LGTM.

P.S.: Please, rebase the main branch.

@Delmael
Copy link
Collaborator Author

Delmael commented Jan 14, 2025

Hi @ajtudela
Thanks !
I've taking your remarks into account and updated the code !
Should be ok to merge

@ajtudela
Copy link
Owner

Hi,
Could you rebase main one last time?

I want to run the testing action to check if everything is ok.

ajtudela and others added 5 commits January 15, 2025 09:21
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
@Delmael
Copy link
Collaborator Author

Delmael commented Jan 15, 2025

Hi @ajtudela
It's done
For information, i will not be able to merge this PR as I donnot have the write access.

@codecov
Copy link

codecov bot commented Jan 15, 2025

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@ajtudela
Copy link
Owner

Hi, the coverage down to 77%.

Could you add more test?

Thanks

@ajtudela
Copy link
Owner

Hi @ajtudela
It's done
For information, i will not be able to merge this PR as I donnot have the write access.

Don't worry. I'll merge it when you added the rest of the tests 😉

@Delmael
Copy link
Collaborator Author

Delmael commented Jan 15, 2025

Hi @ajtudela
It's done
For information, i will not be able to merge this PR as I donnot have the write access.

Don't worry. I'll merge it when you added the rest of the tests 😉

I can work on it 😃

On my local, the coverage of tox is around 90%
---------- coverage: platform darwin, python 3.13.1-final-0 ----------
Name Stmts Miss Cover

custom_components/smartbox/init.py 46 5 89%
custom_components/smartbox/climate.py 115 3 97%
custom_components/smartbox/config_flow.py 79 50 37%
custom_components/smartbox/const.py 27 0 100%
custom_components/smartbox/entity.py 16 16 0%
custom_components/smartbox/model.py 273 3 99%
custom_components/smartbox/number.py 34 0 100%
custom_components/smartbox/sensor.py 125 1 99%
custom_components/smartbox/switch.py 87 0 100%
custom_components/smartbox/types.py 4 0 100%

TOTAL 806 78 90%
Coverage XML written to file coverage.xml

Do u know how can i user codecov in local ?

@ajtudela
Copy link
Owner

ajtudela commented Jan 15, 2025

Do u know how can i user codecov in local ?

I think is not possible. I use LCOV locally instead.

You can see here the patch coverage: https://app.codecov.io/gh/ajtudela/hass-smartbox/pull/5

@ajtudela ajtudela merged commit 3c6eec0 into ajtudela:main Jan 15, 2025
5 of 6 checks passed
@ajtudela
Copy link
Owner

Great job!!

Thanks for the help @Delmael . Feel free to open more PR!

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.

2 participants