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

Add new settings (fanLevel, verticalSwing and horizontalSwing) #74

Merged
merged 4 commits into from
Mar 8, 2024
Merged

Add new settings (fanLevel, verticalSwing and horizontalSwing) #74

merged 4 commits into from
Mar 8, 2024

Conversation

EtienneSOU
Copy link

Hello !
First of all, thank you for the amazing job you do !
I'm using home assistant with Tado and it uses your library, so it's very handy.
Unfortunately, I'm not able to control my AC with Tado new settings which are fanLevel, verticalSwing and horizontalSwing.

I made some small changes to use these if the user needs it.

Thanks !

@Xlinx64
Copy link

Xlinx64 commented Mar 6, 2024

Hi @palazzem @wmalgadey
Could you maybe have a look at this PR?

Copy link
Collaborator

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

@EtienneSOU thank you very much for taking the time to make the change, that's really appreciated (and welcome for your first contribution)! I left only a small comment for you!

That said, I would like to share some thoughts I have about this library with everyone as I think we should discuss it somewhere:

  • I'm not using the library directly, so it's very hard for me to understand if this change is correct, or if it requires more thinking to make the code even better. For instance, I don't know if this issue is impacting everyone or just few newer or older models, or if this issue happened as a consequence of Tado changing their APIs behavior.
  • Unfortunately, even though this library is helping a lot of developers, it's missing a lot of testing (unittests, integration tests, etc...), so it's hard to understand if any merged change is a breaking change or not.

Overall, I'm not against merging this code, but I think we should take an extra effort to rethink how this library is maintained, so that new contributors can have an easy life to commit their changes.

This is what I propose:

  1. This change seems adding something new. @EtienneSOU I would ask you to add testing, but as there are no unittest, this will become a very complex task. For now, let's accept the fact to merge code that can't be tested in the current conditions.
  2. I will try in the next 2-3 weeks to free some of my time to rethink the maintenance of the library, and see what are the steps we can take to improve the situation.

Volunteers are welcomed! It's not needed that you are an experienced software engineer. Even knowing:

  1. Where is this library used?
  2. Are there other successful forks?
  3. Are there some major issues we should focus on?

@Xlinx64 I'm randomly pinging you as you joined the conversation. Maybe you have some clues about the questions I wrote? cc @wmalgadey in case you have.

PyTado/interface.py Outdated Show resolved Hide resolved
@wmalgadey
Copy link
Owner

wmalgadey commented Mar 7, 2024 via email

@EtienneSOU
Copy link
Author

Would love to help maintain this library !
I'm not a pro developer but this could be a great experience.

@Xlinx64
Copy link

Xlinx64 commented Mar 7, 2024

Thank you for your honest comment @palazzem!
I agree that this library could deserve some love and tests. Unfortunately my knowledge in python and the tado api is limited. I just got my first tado device yesterday. I will have a look if I can help somewhere but I am not sure.
This library is used by 3,5% of home assistants users.
Maybe one of the maintainers of the home assistant integration is interested in helping out with this library?
@chiefdragon, @erwindouna ?

@palazzem
Copy link
Collaborator

palazzem commented Mar 8, 2024

Thank you very much, @EtienneSOU, for addressing the feedback! Any help you may provide will be very welcome, so don't think too much about being or not being a "pro developer." Everyone (really, everyone) started with experience == 0 and grew year after year. As you mentioned, it could be a great experience anyway! The only thing that is really important is that you'll have fun while contributing to an open source project! All the rest is secondary 🎉

@Xlinx64, that could be an interesting insight, as 3.5% of HASS users using this library means that around 9327 users are actively relying on this library/fork (source: Home Assistant analytics). It means we should probably make this fork official, without losing the credits to whoever contributed back in the days! Anyway, don't worry, any help you may provide is welcome too! Don't feel the pressure to do something; any second you spend on an open source project (whether it's fixing an issue or filling a bug fix report), is a second you donate to the community!

As a tentative effort to do something, I'll start a new conversation in a new issue where I will ping you back. I'll write down some actions we may take (I will take most of them at the beginning) to improve the situation in the mid-term.

In the meantime, I think we may merge this PR as it's adding new parameters that, if not provided, will still behave as before (it's a safe merge).

@palazzem palazzem merged commit 6cace37 into wmalgadey:master Mar 8, 2024
5 checks passed
@palazzem
Copy link
Collaborator

palazzem commented Mar 8, 2024

I filled a new GitHub issue where we can continue this discussion! #75

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.

4 participants