Move code for exporting attributes to the attribute's classes #82

dragomirecky · 2024-12-06T17:00:40Z

Hello,

this PR makes some relatively small improvements (in my opinion) to the internal structure of the code.

It renames all the DbusSomething classes to DbusAttribute.
- "Something" referred to method/signal/property of an interface. Looking at sdbus and elsewhere, those are usually called "attributes" of the interface. Together with the other changes in this PR, I believe it makes the code clearer.
It renames the classes for bound attributes
- Discussed here already Use bound instead of binded #65
- I believe calling the attributes "bound" is correct, the same way we have "bound methods" in Python. Thus the base classes for bound attributes is "DbusBound".
- The concrete subclasses representing bound attributes are then called DbusLocalMethodAsync, DbusLocalMethodAsync, etc (as suggested by @igo95862 ; not repeating the "Bound" word, as it doesn't seem to be necessary in that context)
Most importantly, it moves some attribute-specific code from DbusInterfaceBaseAsync to the DbusLocalMethodAsync.
- This change is my main motivation behind this PR. By having the logic of "exporting an attribute to dbus" in the attribute class itself, it gets much easier for clients of this library to extend the library with more advanced attributes (which is my case – I need to be able to implement custom versions of @dbus_property_async and this change makes it much easier to do so).
- At the same time, it tries to make the code actually simpler (also slightly improving DbusExportHandle).

igo95862 · 2024-12-06T18:44:03Z

Hello @dragomirecky

Thank you for making this merge request but I have several objections.

Using "attribute" would be very confusing as it is used in D-Bus specification as alternative name for properties. It is also used in the context of XML introspection.
I actually have a plan to replace the existing DbusSomethingCommon, DbusSomethingSync and DbusSomethingAsync with a new common base. (maybe named DbusMemberBase)

I need to be able to implement custom versions of @dbus_property_async and this change makes it much easier to do so

I highly don't recommend making properties advanced as currently libsystemd assumes properties are lightweight.

systemd/systemd#18206 (comment)

igo95862 · 2024-12-06T18:49:22Z

src/sdbus/dbus_proxy_async_interface_base.py

+    def append(self, item: Closeable) -> None:
+        self._items.append(item)
+
+    def close(self) -> None:


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.

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.

dragomirecky · 2024-12-07T23:39:36Z

Using "attribute" would be very confusing as it is used in D-Bus specification as alternative name for properties. It is also used in the context of XML introspection.
I actually have a plan to replace the existing DbusSomethingCommon, DbusSomethingSync and DbusSomethingAsync with a new common base. (maybe named DbusMemberBase)

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.

I need to be able to implement custom versions of @dbus_property_async and this change makes it much easier to do so

I highly don't recommend making properties advanced as currently libsystemd assumes properties are lightweight.

systemd/systemd#18206 (comment)

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 reactivex.subject.BehaviorSubject). Or have value mapping functions for going to dbus/from dbus.

igo95862 · 2024-12-08T00:18:39Z

That is a very good point 👍 . I like the DbusMemberBase/DbusMemberCommon. I will update the PR if you are ok with it.

Could you also separate these in to a different pull request?

You can make this PR a stacked on top of new PR.

dragomirecky · 2024-12-09T00:27:16Z

Rebased on top of the other PR and fixed the linter warnings.

igo95862 · 2024-12-14T15:22:44Z

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.

dragomirecky · 2024-12-16T16:09:16Z

Still not entirely convinced about point 3.

Is there some way how I can convince you? :)
As stated, the third point is the entire motivation behind this PR. It makes the library usable for my project and it makes all the difference to lot of use cases I use it for (until then, I will have to be maintaining those or similar changes in my fork).

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.

Do you see some downside in merging it before you start adding those changes? Or some downside in merging it in general?

igo95862 · 2024-12-16T19:33:10Z

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.

It makes the library usable for my project and it makes all the difference to lot of use cases I use it for

Can you give an example of what is currently not possible?

dragomirecky · 2024-12-17T11:30:21Z

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.

The library version is 0.13.0 and the README states "Python-sdbus is under development and its API is not stable.". How does this PR change anything about this?
In case you meant "append_to_interface", I just renamed it to be prefixed with underscore to make it explicitly private. Is there anything else you are concerned about?

It makes the library usable for my project and it makes all the difference to lot of use cases I use it for

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:

having a dbus property decorator backed by something like reactivex.BehavirorSubject
having a dbus method decorator, that handles transforming DBus errors to Python errors back and forth (in ways this library does not allow)
having a dbus property decorator, where it is possible to specify mapping functions (for example to automatically convert Enums to dbus values and back)

igo95862 · 2024-12-18T11:37:00Z

for example to automatically convert Enums to dbus values and back

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.

igo95862 · 2024-12-20T14:06:31Z

having a dbus method decorator, that handles transforming DBus errors to Python errors back and forth (in ways this library does not allow)

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 @dbus_method_async to have the data pass through your modification layers:

@dbus_method_async(...)
@my_error_handling(...)
async def my_method(self):
   ...

igo95862 reviewed Dec 6, 2024

View reviewed changes

dragomirecky force-pushed the structure-improvements branch from 773e34a to 03ae03f Compare December 9, 2024 00:15

dragomirecky mentioned this pull request Dec 9, 2024

Rename DbusSomething* and *Binded types #83

Merged

dragomirecky changed the title ~~Improve naming and move code for exporting attributes to the attribute's classes~~ Move code for exporting attributes to the attribute's classes Dec 9, 2024

dragomirecky force-pushed the structure-improvements branch from 03ae03f to 02f9fcb Compare December 16, 2024 16:03

dragomirecky force-pushed the structure-improvements branch from 02f9fcb to e3d5192 Compare December 17, 2024 11:30

Move code exporting attributes to the attribute's classes

68884ba

dragomirecky force-pushed the structure-improvements branch 2 times, most recently from 82806c1 to 68884ba Compare December 17, 2024 12:48

Uh oh!

Move code for exporting attributes to the attribute's classes #82

Are you sure you want to change the base?

Move code for exporting attributes to the attribute's classes #82

Uh oh!

Conversation

dragomirecky commented Dec 6, 2024

Uh oh!

igo95862 commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igo95862 Dec 6, 2024

Choose a reason for hiding this comment

Uh oh!

dragomirecky Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

dragomirecky commented Dec 7, 2024

Uh oh!

igo95862 commented Dec 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dragomirecky commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igo95862 commented Dec 14, 2024

Uh oh!

dragomirecky commented Dec 16, 2024

Uh oh!

igo95862 commented Dec 16, 2024

Uh oh!

dragomirecky commented Dec 17, 2024

Uh oh!

igo95862 commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igo95862 commented Dec 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

igo95862 commented Dec 6, 2024 •

edited

Loading

igo95862 commented Dec 8, 2024 •

edited

Loading

dragomirecky commented Dec 9, 2024 •

edited

Loading

igo95862 commented Dec 18, 2024 •

edited

Loading