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

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

Merged
merged 2 commits into from
May 25, 2020

Conversation

GeorgeSG
Copy link
Contributor

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:

  • HTTPAirClient
  • CoAPAirClient
  • Version107Client

CLIs:

  • HTTPAirCli
  • CoAPAirCli
  • Version107Cli

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

Misc changes

  • Added .gitignore because I used venv to develop this
  • Added .pylintrc
  • Resolved some lint errors, disabled a lot of them (figured they can be resolved later)
  • Formatted the new classes with black. Let me know if you prefer some other formatting.

Todo and next steps

  • Extract CloudClient from cloudctrl as a client
  • Maybe use status_transformer in other CLIs? Currently used in v107 only
  • I didn't change the ~/.pyairctrl file logic. It's in the HTTP Client now. Maybe it should be optional or moved to the CLI? Not sure pure clients need this.

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

@GeorgeSG
Copy link
Contributor Author

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

@rgerganov
Copy link
Owner

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
Copy link
Contributor

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

  • The HTTPAirClientBase (with should be renamed btw), was meant to be a general baseclass to avoid code duplication, since all 3 versions are similar from that point where we have a valid json.
  • This baseclass uses status_transformer, so it will be used everywhere we inherit from it.

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

@Cyber1000
Copy link
Contributor

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
Copy link
Contributor

Ok looked through your branch:

  • I think I wouldn't separate cli and classes this way as we can refactor quite a lot within our current client-classes (see my draft)

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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor

  • Well that sounds valid to me, you'll get your json within your clients and do the formatting within the cli-classes
  • I've tried your branch with v1.0.7 and it works for me now 👍
  • Maybe someone with version 0.2.1 (@shexbeer ? ) could test your version too.
  • Some tests would be useful in the future (before the next cleanup in airctrl.py and baseclass changes), I'll take a look into this Add (unit)tests #43 after this PR
  • The split into different client-modules seems valid as this gets more readable

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
Copy link
Contributor Author

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

My purifier uses 0.2.1 and it works.

if debug:
self.coapthon_logger.setLevel("DEBUG")
status = self._get()
status = self._client.get_values(debug)
Copy link
Owner

Choose a reason for hiding this comment

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

this should be get_status instead of get_values. Will commit your PR as is and will fix it in a later patch.

@rgerganov rgerganov merged commit b32f007 into rgerganov:master May 25, 2020
@rgerganov
Copy link
Owner

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:

  • HTTP (my initial work)
  • CoAP with encryption (contributed by @Cyber1000 )
  • CoAP without encryption (contributed by @shexbeer )

@GeorgeSG GeorgeSG deleted the refactor-as-cli branch May 26, 2020 16:40
@Cyber1000 Cyber1000 mentioned this pull request Jun 1, 2020
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

3 participants