Bouni / python-luxtronik

python-luxtronik is a library that allow you to interact with a Luxtronik heatpump controller.
MIT License
37 stars 20 forks source link

Add dump functionality #59

Closed Bouni closed 1 year ago

Bouni commented 1 year ago

A lot of users ask how to find certain values and I always point to a gist where I put a script that does this.

Maybe we should integrate this functionality into the module itself so that we can cal it like python luxtronik dump or so.

kbabioch commented 1 year ago

I would rather suggest to add a scripts directory to the repository and put the script there, e.g. something like scripts/dump-luxtronik.py. To my understanding this is a script making use of the library and not a classical Python module.

Bouni commented 1 year ago

Suggesting python luxtronik dump was by intention.

That way a user can simply install the module with pip install luxtronik and then call the function. If we just bundle the script the user cannot simply call it without knowing the install path, right?

Maybe thats the first step towards some kind of CLI (not sure if that makes sense tbh)

kbabioch commented 1 year ago

Having a CLI later is a great idea, i.e. you could interactively change some values and get the look and feel like sitting in front of the controller.

Regarding having this as a Python module: Personally, I don't like that too much. I expect users of a Python library to locate a script and run it. Also its a question of packaging, i.e. you could easily put the script into PATH, so you don't have to look for it.

AFAIK most Python libraries provide dedicated scripts, rather than Python modules than are imported and run some magic. Besides that: How would you specify IP address and port when running python luxtronik dump?

Bouni commented 1 year ago

I just put to gether a really quick and dirty cli-draft:

The essential changes are here: https://github.com/Bouni/python-luxtronik/blob/cli-draft/setup.py#L17 And here: https://github.com/Bouni/python-luxtronik/blob/cli-draft/luxtronik/__main__.py

With this you pip install the luxtronik module and can run a luxtronik --ip 192.168.88.11 --port 8889 dump without the need for python -m or similar as prefix.

Disclaimer, I've never really used argparse, maybe there's a more elegant solution for this.

kbabioch commented 1 year ago

While #61 addresses the issue, I don't think we had discussed and aligned the route here yet. So I'm re-opening this issue in order to revisit it.

Personally I think scripts along with the package are "good enough", but I can also see some advantages in the proposal from @Bouni, so I'm not against it and also fine with getting rid of my pull request #61 in favor of another solution.

In a world with enough time, I would also like to implement a real CLI application based on this library, i.e. something based on ncurses or similar that can be used to get the look and feel of being infront of the controller physically. Unfortunately there is too much projects and too little time ;-).

Bouni commented 1 year ago

I'm still in favor of my proposal of having a decent CLI. No need to be full fledged but having discovery and dump functionality would be a good start.

We could still have the scripts in a script folder like in the already merged #61 and have my proposal that simply imports them on a given CLI call.

What do you think of that?

kbabioch commented 1 year ago

I can agree to that. On the other hand for me this is not the biggest priority. Since we don't have an official roadmap, we'll have to wait for someone of us to work on this ;-).

Bouni commented 1 year ago

There's one little obstacle with this. The current folder structure looks like this:

.
├── dist
├── luxtronik
├── luxtronik.egg-info
├── scripts
└── tests

This means we cannot import scripts/dump-luxtronik.py in luxtronik/__main__.py because scrips is not a module, not within luxtronik and most important, when installed via pip, not installed at all.

I suggest that we move the scripts folder into luxtronik and add a init.py to it. That way we can import the scripts and they get installed via pip.

kbabioch commented 1 year ago

@Bouni can this be closed now with #83 being merged?

Bouni commented 1 year ago

I think so, yes!