Improve library reliability and quality (path to 1.0)

[palazzem]

Intent

Hello everyone!

This post aims to support those interested in contributing to this remarkable library, whether by fixing bugs or adding new features. I hope to keep this issue open until the library meets all criteria for being considered well-tested and reliable. This is a crucial step, especially given its widespread use, including by 3.5% of active Home Assistant users and many developers working on it independently.

Moreover, some contributors may wish to address a bug or even report one, but without clear guidance and support, starting on the project can be daunting. We need to take steps to lower these barriers and facilitate contributions. @wmalgadey has been doing incredible work here, yet time is finite, and we may offer some level of support. Of course, there's no pressure to contribute!

Let's explore some baby steps we can take to enhance this library over time.

Path to 1.0

Our initial steps might include:

1. Establishing contribution guidelines, tooling, and measure our progress towards our stability goals.
2. Increase code coverage to prevent regressions or breaking changes.
3. Ensuring Home Assistant Tado integration is well supported on features/fixes they need.

For Point (1), we could undertake the following tasks:

• Update repository settings to establish some rules (merging, releases, actions, milestones, roadmap, etc.) #106
• Create GitHub templates to assist contributors in reporting bugs or submitting PRs #105
• Publish contribution guidelines to help newcomers with their first contributions (code standards, etc.) #104
• Implement tooling (e.g., pre-commit hooks) to maintain standards and simplify the review process #103
• Introduce coverage tools #102

For Point (2), we need contributors to start by writing at least one test for every new change. Although backfilling tests is beneficial, to me is far important that new fixes and features are covered by tests. We should use a "build over time" approach, no need to go heads down to eventually lose motivation and fun.

Point (3) is unknown to me at the moment, as there are known issues (some of which I've encountered), but it's unclear whether the solutions should be implemented in this library or directly within the integration.

Who can contribute?

Anyone with time to spare is welcome! Filling bug reports, performing code reviews, fixing bugs, writing documentation, communicating with HA Tado Integration contributors, etc., are just a few ways you can help.

As the proposer of these steps, I'm eager to take most of them, particularly all tasks related to Point (1) and some of Point (2).

How can I give feedback?

Please respond below! This post is a proposal, and feedback is what transforms a proposal into a strong one. So don't hesitate to contribute with your thoughts, and thank you in advance for any suggestions you may have!

[palazzem]

@wmalgadey if possibile and whenever you are back / have the time, can you give me admin permission to this repository so that I can change some repository settings? That will unblock me to do some initial changes!

In the meantime I'm adding @Xlinx64 and @EtienneSOU who joined a previous conversation around this, and may be interested in helping (again, no rush and no pressure!).

In terms of actions, as soon as I complete point (1), I think external contributors may have an easier life in writing new tested changes. For point (3) I will rely on anyone who has ideas on what are the most important bugs / features to address (we can broadly discuss them here, and eventually fill a bug report). I actually don't know what is the state of the Home Assistant integration, and if these bugs are related to this library or the integration itself.

Thoughts?

[Xlinx64]

On (3) I am not sure. It not easy to check if it is an integration or library problem. It has to be checked on an issue to issue basis.
I can only speak for my issues.
I used the tado integration a long time without any issues for my radiator valves. But now I dont have them anymore and just have a tado AC controller. With this I have a few issues. Missing swing and fan levels, which have to be fixed in the home assistant integration after the change from @EtienneSOU get released from this library. I also cannot turn on the fan only mode. But not sure where the issue lies.
I am still trying to learn about the api and this library to reproduce the fan only issue.

PS: @palazzem I just noticed that you opened an issue for this 3 years ago xD
#47

[palazzem]

Yeah I agree, we can take the third point case by case! Unfortunately as you are saying, it's very hard to identify where the problem lies. I found the issue #47 a while ago. I did some changes on the integration to make it work, but it never landed in HA codebase as my fix was breaking someone else behavior. So I decided to use Tado integration as a custom integration with the fix. Then, a year ago roughly, I've started using again the core integration and it was working (somebody did a change that fixed my case probably). At the moment, all radiator valves and AC devices are working fine, even though that wasn't the case 1-2 years ago.

Anyway, I'd say we can make progress by increasing the testing so that when an issue like the one you are experiencing is fixed, it stays fixed in the long-term.

About @EtienneSOU change, I think I can ship a release, probably tomorrow or Monday as soon as I'm back home! Let's stay in touch in this thread as I'm freeing some time to help maintaining a bit this library (and maybe the integration too!).

[wmalgadey]

@palazzem github workflow will build a release and push it to pypi if you tag it and create a github release. Adding the correct release in setup.py is just an addition, but not necessary.

[palazzem]

Thanks @wmalgadey for the heads-up! @Xlinx64 I shipped the release: https://pypi.org/project/python-tado/0.17.5/ . I guess you should notify whoever is fixing your issue in the HA integration, right?

@wmalgadey whenever you have time, let me know what you think about my plan. If you can grant me admin permissions on your repo, I may start making some major changes to simplify testing and external contributions. Thank you very much in advance!

[EtienneSOU]

Thanks for the release @palazzem
I’ll check if I can submit a PR to HA.

[wmalgadey]

I don‘t find where :) you are already contributor

[albertomontesg]

Is also within the goal to make the library async so it can be integrated more natively with the async engine in Home Assistant? I would be happy to contribute to this (as well as some other points previously exposed)

[Xlinx64]

Hey @palazzem !
There is already a great contribution from @albertomontesg concerning the path to 1.0.
Also @EtienneSOU made another contribution to fix the current issue with Tado Ac Controllers in Home Assistant: #77
Could you maybe please take a look at it?

[wmalgadey]

@Xlinx64 I merged #77 and create 0.17.6

I took the changes from #79 into a local branch and fixed the merging conflicts. See #83

If we agree with the changes made by @albertomontesg, we could merge it asap.

[wmalgadey]

Sorry for the mess! I had to delete 0.17.6 and some commits I made by mistake. I used the wrong email-Adress in my commits, and have to erase those!

[wmalgadey]

Again so sorry for this mess. I hope everything is again back to normal :D

#84 will now replace #79

[wmalgadey]

@palazzem and @albertomontesg I did add your changes for code improvement and testing to the main branch. I also did refactor the implementation for the Tado X-Api changes (FYI @Moritz-Schmidt).

If you find some time, please take a look and I will release this

[palazzem]

@wmalgadey that's great! thank you very much for the time you are spending in this project! I got side tracked on another one, but I'd like to contribute again in the near future! In the meantime, do you want me to review https://github.com/wmalgadey/PyTado/releases/tag/0.18.0 release? I can check what has been merged in case I find something!

[wmalgadey]

@palazzem I already made some changes for 0.18.1 (found some bugs :/), but yeah would be nice if somebody could take a look

[wmalgadey]

I did add some github features, like issue templates, pr templates and activated "all" security features. I also did add code coverage, but couldn't integrate it properly into github!

[wmalgadey]

testresults are now displayed in github action summary and as a sticky comment in each PR. I configured the threshold to 60-80% code coverage.

[palazzem]

I finished reviewing all the latest changes and I'd say everything is great! With these steps you are improving a lot the quality of your repository, by providing metrics (e.g. code coverage and other tools) that helps contributors in making their PR more stable and reliable. I've already mentioned in another step, that to merge contributors code we may require at least one regression test to ensure this behavior doesn't ever break again (we're slower in short-term but faster and reliable long-term).

Other than that, I saw the API moved to a snake-case naming convention a while ago, and we have some deprecation warnings (super great!). One thing we can do better, is eventually tell users WHEN the method will be removed (e.g. "The 'getMe' method is deprecated and will be removed in version 2.0, " "use 'get_me' instead"). So when 2.0 happens (random version, it can be anything), with a single PR you'll remove these methods. This accepted in OSS communities, even though we should give time to our users to migrate, so it should not be a version released in the near future.

I will write more as soon as I have more thoughts to share!

[wmalgadey]

@palazzem I removed the pascalCase methods already with 0.18.x, because it was part of the PR I recreated. Should I add the methods back again?

[palazzem]

@wmalgadey ah I see, I missed that commit probably! Considering the library has been shipped, we have two options:

1. We bite the bullet and we hope that people will spend time with the migration, so they can use the latest version
2. We reintroduce them back in a bugfix release, so that everyone can upgrade to the latest version without changing their code

The challenge I see here, is that I think the latest release has a lot of fixed stuff thanks to your/contributors work, and to be sure it's propagated to all projects, it may be faster if they can just upgrade without spending time to change their code.

My recommendation is to revert the PR and ship a bugfix release. Then we'll take our time and decide when it's a good time to drop these APIs. What do you think?

[wmalgadey]

@palazzem see #114. I added the pascalCase methods back again and will release 0.18.5 with this change :)

[palazzem]

Great! We'll think about removing it in one of the coming major releases! No rush as it's not that critical right now! Thank you so much!

[wmalgadey]

@palazzem I'll close this issue. Everything you mentioned is done and the rest has moved to new issues. I personally think, we are on a good way here ;)

[palazzem]

Well done @wmalgadey ! That's a great improvement for sure!