Bouni / python-luxtronik

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

Added a decorator to connect to the heat pump #131

Closed Guzz-T closed 6 months ago

Guzz-T commented 1 year ago

Using a decorator we can simply re-use the code to connect to the heat pump. This allows us to offers several read / write functions like only read the paramters.

Fixes #130

gerw commented 1 year ago

Shouldn't it be possible to use your _connect as a "real" decorator (with @) around the read/write methods?

Guzz-T commented 1 year ago

Unfortunately, a "real" decorator cannot use self as an argument. Without self, however, the internal fields cannot be accessed correctly. Also pylint complains.

gerw commented 1 year ago

I tried a little bit and it seems to be possible with a "real" decorator:

def decorator(func):
    def _decorator(self, *args, **kwargs):
        self.num_calls += 1
        return func(self, *args, **kwargs)
    return _decorator

class TestSample:
    def __init__(self):
        self.num_calls = 0

    @decorator
    def dummy(self, foo):
        print(foo)

t = TestSample()

print(f"t.num_calls: {t.num_calls}")
t.dummy(2)
print(f"t.num_calls: {t.num_calls}")

There are also no complaints by pylint (only concerning missing docstrings and so on).

Guzz-T commented 1 year ago

Have you tried accessing private fields like self._num_calls instead of self.num_calls?

I previously had the decorator inside the class. If you place it outside - like in your example - you at least bypass the fact that pylint requires a self argument.

gerw commented 1 year ago

Yes, the following is pylint-compatible:

"""Random module"""
def decorator(func):
    """Random decorator"""
    def _decorator(self, *args, **kwargs):
        self._num_calls += 1
        return func(self, *args, **kwargs)
    return _decorator

class TestSample:
    """Random class"""
    def __init__(self):
        self._num_calls = 0

    def another_method(self):
        """Random member fctn"""
        return self._num_calls

    @decorator
    def dummy(self, foobar):
        """Random member fctn"""
        print(foobar)

t = TestSample()

print(t.another_method())
t.dummy(2)
print(t.another_method())
kbabioch commented 1 year ago

@Guzz-T Could you elaborate on what the vision here is? How do you envision to use this in the future?

github-actions[bot] commented 1 year ago

Coverage

Coverage Report
FileStmtsMissCoverMissing
luxtronik
   __init__.py13410621%38–54, 61–66, 69–73, 79, 83, 102–113, 116–119, 122–142, 145–160, 163–180, 183–198, 202–204, 208–209, 213–214
   __main__.py21210%2–48
   datatypes.py2351295%37, 42, 47, 57, 72–75, 80–83, 92
   discover.py433421%25–77
luxtronik/scripts
   dump_changes.py44440%5–93
   dump_luxtronik.py28280%5–64
TOTAL58824558% 

Tests Skipped Failures Errors Time
110 4 :zzz: 0 :x: 0 :fire: 0.637s :stopwatch:
Guzz-T commented 1 year ago

@Guzz-T Could you elaborate on what the vision here is? How do you envision to use this in the future?

@kbabioch: Does the comment in #130 answer your question?

Guzz-T commented 1 year ago

Yes, the following is pylint-compatible:

@gerw: I followed your suggestions, but now we get several W0212 pylint warnings

gerw commented 1 year ago

This is strange. Pylist does not complain about something like self._num_calls += 1, but about self._num_calls = self._num_calls + 1.

Guzz-T commented 1 year ago

This is strange. Pylist does not complain about something like self._num_calls += 1, but about self._num_calls = self._num_calls + 1.

I reverted the changes because it would just be a different style of the decorator

gerw commented 1 year ago

Since we are not saving the decorated function, it seems not necessary to implement this as a decorator. What about something like

    def _with_lock_and_connect(self, func, *args, **kwargs):
        with self._lock:
            is_none = self._socket is None
            if is_none:
                self._socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
            if is_none or is_socket_closed(self._socket):
                self._socket.connect((self._host, self._port))
                LOGGER.info(
                    "Connected to Luxtronik heat pump %s:%s", self._host, self._port
                )
            return func(*args, **kwargs)

    def read(self):
        return self._with_lock_and_connect(self._read)

?

Guzz-T commented 1 year ago

There isn't just one way of doing it. My version works like a "@"-decorator.

Guzz-T commented 7 months ago

Is there still interest in these changes?