search

rgerganov / py-air-control

Command line app for controlling Philips air purifiers
MIT License
266 stars 53 forks source link

Implements #34: Split into Client and CLI classes #41

Closed GeorgeSG closed 4 years ago

GeorgeSG commented 4 years ago

Notice

I have an old purifier that uses HTTPAirClient. I've tested that and it looks like it's working. I have no way to test the other clients / CLIs. It would be awesome if someone else can try this and verify that it works :)

Summary

Initial version of splitting pyairctrl into Client and CLI Classes.

Clients:

CLIs:

Basically, CLIs act as a wrapper for each client and add printing of responses (dump_status, etc). airctrl now uses CLI classes.

Misc changes

Todo and next steps

In general, I think this is a good first step to split up the codebase, but definitely could use more work.

GeorgeSG commented 4 years ago

I understand this is a big change, maybe #40 is the safer choice for now :)

rgerganov commented 4 years ago

Thanks for starting this!

Splitting the clients into separate modules looks fine but I am not sure about the client package. Having "client" in both the package name and the module name doesn't look very nice. Let's follow the Python principle of "flat is better than nested" and get rid of the client package.

I am also not sure about the CLI modules and classes. There is a lot of code duplication there and I feel that we should be able merge all of those after some refactoring. I suggest to keep this stuff in airctrl.py for now and see how we can refactor this after splitting the clients.

What do you think?

Cyber1000 commented 4 years ago

Just wanted to add my two cents since I added the v107 part some days ago:

I think I'll make a quick draft about my intital intention of this baseclass.

Cyber1000 commented 4 years ago

Just a quick draft is here #42, which won't work (except for 1.0.7), just to have an idea of the base class

Cyber1000 commented 4 years ago

Ok looked through your branch:

btw, I'm getting this for now when I run your branch with 1.0.7 (using python 3.6, if that matters) image

Now the difficult part: How to get all this together ... 😄 Maybe smaller steps ...

Cyber1000 commented 4 years ago

ok I think that this is due to something not really installed with "python3 setup.py install", it can't find the cli-files.

Ok enough for today ...

GeorgeSG commented 4 years ago

@rgerganov, moved the clients out of the package and moved the CLIs in airctrl.

@Cyber1000, yeah I figured that was the idea with the HTTPAirClientBase, but didn't want to change that with this PR

GeorgeSG commented 4 years ago

It makes sense to me to have a CLI class on top of the client. The client classes shouldn't be worried about printing and formatting data, just returning it.

I agree the CLIs are very similar right now and can be refactored more though

Cyber1000 commented 4 years ago

Added .pylintrc

👍

Formatted the new classes with black

I didn't know this before (python is not my main-language), if we keep this maybe a own contribute-section in the README would be nice with hints how to install (for example I use vscode and black seems to integrate nicely with that - https://medium.com/@marcobelo/setting-up-python-black-on-visual-studio-code-5318eba4cd00 ). But that's up to the repo-owner @rgerganov

GeorgeSG commented 4 years ago
  • Maybe someone with version 0.2.1 (@shexbeer ? ) could test your version too.

My purifier uses 0.2.1 and it works.

rgerganov commented 4 years ago

Thanks @GeorgeSG, I have merged your work.

  • Maybe someone with version 0.2.1 (@shexbeer ? ) could test your version too.

My purifier uses 0.2.1 and it works.

It appears that firmwares with the same version could be using different communication protocols. I am going to replace the version argument with protocol and users would be able to choose between the following protocols: