Move code for exporting attributes to the attribute's classes #82
Conversation
|
Hello @dragomirecky Thank you for making this merge request but I have several objections.
I highly don't recommend making properties advanced as currently |
| def append(self, item: Closeable) -> None: | ||
| self._items.append(item) | ||
|
|
||
| def close(self) -> None: |
There was a problem hiding this comment.
Not sure why the close method is needed. The context manager is already implemented for DbusExportHandle.
https://docs.python.org/3/library/contextlib.html#contextlib.closing
Most types managing resources support the context manager protocol, which closes thing on leaving the with statement. As such, closing() is most useful for third party types that don’t support context managers
Also close might be misleading because the bus objects use it to close the connections to D-Bus but here it does not actually close the D-Bus.
There was a problem hiding this comment.
- I made the DbusExportHandle a bit more generic; you can insert any type into it, which has the standard Python's close() method. Which fits nicely with the SdBus slot, which also implements this.
- Makes it more Pythonic in my opinion, as most Python types implement both the context manager protocol and the close() method (so it can be easily cleaned-up manually by calling the "close" method, instead of calling
__(a)exit__()if not bound to a context) - Not sure how close() might mislead user to thinking it closes the connection, when the method is being on an "export handle".
If you don't like it, or think the change is not worth it, I am ok with removing it. I can live without it, but it still seems to me as an improvement to the code.
That is a very good point 👍 . I like the DbusMemberBase/DbusMemberCommon. I will update the PR if you are ok with it. At the same time I will fix the linter warnings.
Yep, I am aware. The custom version of @dbus_property_async I mentioned is not about making properties async. But it allows me to, for example, have properties backed by a reactive value (like |
Could you also separate these in to a different pull request? You can make this PR a stacked on top of new PR. |
773e34a to
03ae03f
Compare
|
Rebased on top of the other PR and fixed the linter warnings. |
|
Still not entirely convinced about point 3. I plan on adding a lot of changes in the next version including raising the minimum Python version to 3.9. I will revisit this MR closer to the end of the development of the next version. |
03ae03f to
02f9fcb
Compare
Is there some way how I can convince you? :)
Do you see some downside in merging it before you start adding those changes? Or some downside in merging it in general? |
The issue is I would need to commit to supporting a particular low level API, however, I am not ready to do so. The current low level API is a quick glue between Python and libsystemd. I plan on stabilizing the low level API and documenting it once the high level API will finalize.
Can you give an example of what is currently not possible? |
It makes it possible to create own versions of @dbus_method_async and @dbus_property_async, and to that so by using very little of private/unstable API. There are more use-cases for custom versions of those decorators:
|
02f9fcb to
e3d5192
Compare
82806c1 to
68884ba
Compare
You can already do something like this: from asyncio import sleep, run
from enum import IntEnum
from sdbus import DbusInterfaceCommonAsync, dbus_property_async, request_default_bus_name_async
class FooBar(IntEnum):
FOO = 0
BAR = 1
class Test(DbusInterfaceCommonAsync, interface_name="org.example.test"):
def __init__(self) -> None:
super().__init__()
self._foobar = FooBar.FOO
@dbus_property_async("i")
def foobar(self) -> int:
return self._foobar
@foobar.setter
def _foobar_set(self, new_value: int) -> None:
self._foobar = FooBar(new_value)
async def main() -> None:
await request_default_bus_name_async("org.example.test")
test = Test()
test.export_to_dbus("/test")
while True:
await sleep(600)
if __name__ == "__main__":
run(main())Because the int enum is a subclass of int it will be treated as an int then packed to D-Bus. |
Not sure what you mean "in ways this library does not allow". The only thing missing that I can think of is appending extra D-Bus data to error D-Bus messages. (specification allows that) I plan on adding such feature soon. Also you can probably stack the decorators bellow the |
Hello,
this PR makes some relatively small improvements (in my opinion) to the internal structure of the code.
boundinstead ofbinded#65@dbus_property_asyncand this change makes it much easier to do so).