LEW21 / pydbus

Pythonic DBus library
GNU Lesser General Public License v2.1
327 stars 76 forks source link

Support automatic XML introspection generation #29

Open xZise opened 7 years ago

xZise commented 7 years ago

This enables a class to generate automatic XML introspection data. This is probably not the final version, especially as I noticed that you use tab indentation while I use spaces (the TypeError exception for example is copied and thus uses tabs).

But the main reason for already open this request now is to determine if this is actually needed or wanted.

I unfortunately haven't actually tested this on an actual D-Bus service as I don't actually have written one yet. I'll probably test that as well when there is interest.

LEW21 commented 7 years ago

It's hard to say if it's a good idea or not.

In most cases, the DBus Introspection XML is a part of a pre-existing standard (like MPRIS), and the programmer can simply copy the XML file without needing to worry how to exactly replicate its behavior in his Python code. I guess we could also provide a object validator that automatically checks if all required methods are implemented, and that they take correct number of arguments; or even a XML to Python skeleton converter for quick starting implementation.

However, when there is no pre-existing standard, and the programmer is creating a new one, it can be quite easier to write everything in Python, and automatically generate XML data - as XML is not really a nice format to write by hand.

Also, having the argument data types specified in the Python source code might be useful for its readability. In fact, we could automatically test if the parameters have correct types even when methods are not called through DBus, but from other Python code - which might be useful for autotests, and for objects that are both published on DBus and used internally by the application. This suggests that type-related decorators could be implemented as a generic, non-dbus related tool, like signals in pydbus.generic.

But we don't always need type decorators. Without them, we can generate a skeleton XML file - listing all the methods and their argument names; but specyfing their arguments as "TODO" - to help programmer with writing the XML file.

I guess, we can split this feature into two smaller ones - the one about generic type-related decorators; and the one about generating DBus XML from a given Python class. And the first one should expose metadata that will let the second one automatically fill out the TODOs, and let the user publish the object without touching the XML at all. Both are useful and self-contained, and together they have synergy.

As an added bonus, developers will be able to quickly change from using autogenerated XML to using a XML file when they decide to freeze the API and publish it as a standard; without a need to remove type decorators, as they're still a useful part of the code.

LEW21 commented 7 years ago

OK, so now back to your pull request:

  1. @signalled is a bad idea, as not every call to the setter will change the value (and therefore should emit the signal; for example, we don't want to emit if the new value is equal to the old value); and not always the value is changed by a call to the setter (for example if we're observing some remote / hardware variable). Normal @PropertyName.setter + emitting the PropertyChanged signal manually is a better way here. (pydbus in the future might provide its own property() that's automatically handling getting, setting and emiting the signal; but that's outside of the scope of this feature)
  2. @dbus_method/@dbus_property should be in pydbus.strong_typing module, and should be called @typed_method((arg,...), ret), @typed_property(t) and typed_signal(arg,...). You don't have to implement type checking, I can implement that later.
  3. gen_introspection should live in pydbus.xml_generator module and should be called generate_introspection_xml(require_strong_typing=False); it should be imported into the main pydbus module. It should generate XML introspection for all members that start with an uppercase letter and are not in ("PropertiesChanged", "Get", "GetAll", "Set"). For the strongly typed ones, it should output types to the XML; for untyped ones it should use "TODO" instead
  4. @dbus_interface should also live in pydbus.xml_generator module and be imported into main pydbus module. I'm not sure how it should be called, as I don't want people to think it's the recommended way to provide XML introspection; but on the other hand, it shouldn't be too complicated. I lean toward @autogenerate_introspection_xml(interface_name).

Note that it should work in the same way as

class xxx(object):
    ...

xxx.dbus = generate_introspection_xml(xxx, require_strong_typing = True)

5) It would be awesome if you could put the strong_typing-related things in one commit, and generate_introspection_xml-related things in a second one.

xZise commented 7 years ago

Okay I'll look more closely through your suggestions but thanks for your response. I have fixed the tab/spaces issue, so future commits will be fixed (I'm going to change this commit here too).

Regarding @signalled (point 1): This is up to the developer, because @dbus_property generates a normal @property so using @foobar.setter will work and you don't have to use @signalled. It is only there so that it automatically populates that XML entry.

And regarding the weakly typed methods and properties (point 3), @dbus_property and @dbus_method still provide the functionality of defining/overriding the interface used.

Also gen_introspection already makes it possible to use it like you suggested by explicitly doing clazz.dbus = gen_introspection(clazz).

LEW21 commented 7 years ago

@signalled - right, I've forgotten about this annotation. So this is necessary, but I still don't think that it should send the signal automatically. And probably should be used on the getter, not the setter.

@dbusproperty/method - I don't really understand. I think we can safely assume that all names starting from an uppercase letter are public, as normal python names start from lowercase letters, and everything magical starts from . If somebody adds a new method starting with an uppercase letter, he probably wants to make it public; and if not, he's writing a really confusing piece of code.

About gen_introspection - yup, I know :D

xZise commented 7 years ago

But to which interface does the method or property belong? With @dbus_interface it could belong to that name, but it is possible to override that behaviour and define to which interface a method/property should belong with the @dbus_method decorator:

@dbus_method(interface='foo.bar')
def Example(self):
    pass

This doesn't mean that it must be mandatory. Via @dbus_interface it is possible to specify a default interface and it should be possible to just add all uppercase-starting-methods. What I mean is that this decorator has still a use, just to override the interface (and not to define any types).

Regarding checking the types I first was thinking whether it could infer the types from the signature:

def Example(self) -> int:
    return 42

But this is afaik not really possible, as int could be mapped to multiple types in D-Bus. Now when you mentioned a tester for this I first thought that this might have similar issues. But you only work in the other way (e.g. you check that Example returns actually an int) and that is probably more easily.

I just remember that I wanted to add “autocomplete” features (e.g. if the interface is .foobar to add org.freedesktop).

LEW21 commented 7 years ago

Objects implementing multiple interfaces are a bit of a complicated case. There are cases like MPRIS2, where all methods are grouped into 4 interfaces, and 3 of them are extensions; and there are cases where they're used for versioning, or dynamically added and removed based on some events (like a optical drive, that adds an interface when a CD is inserted).

In the MPRIS case, I guess annotating decorators are the way to go. The other two cases aren't currently supported by pydbus in any sane way, so they're out of scope here.

So, we have two ways to specify MPRIS's ActivatePlaylist method:

1.

@dbus_method(("o"), interface="org.mpris.MediaPlayer2.Playlists")
def ActivatePlaylist(self, playlist_path):
    ...

2.

@interface("org.mpris.MediaPlayer2.Playlists")
@typed_method(("o"))
def ActivatePlaylist(self, playlist_path):
    ...

I feel the second one is prettier, as we're stating two different facts about the method; and the "at interface" sounds semantically well.

The @interface decorator of course can be used also for properties and signals.


However, while thinking about it more, I've found another possibility:

3.

@interface("org.mpris.MediaPlayer2.Playlists")
def ActivatePlaylist(self, playlist_path: "o"):
    ...

And this is the cleanest option. However: Python 2 does not support annotations :D But while we could provide a Python 2 fallback (2nd option, with @typed_method renamed to @annotate_method, and without type checking), this is a place where we're clearly using a Python 3 feature, and we can direct Python 2 users to the existing, XML file based method of specifying interfaces. And I feel there're already too many hacks for Python 2 in pydbus, and it's time to give people reasons to migrate to Python 3.

In the future, @typechecked decorator could be added to verify the annotations at runtime, but it's not really required now, and outside of scope of this feature.

Also, as it's really easy to annotate methods this way, I don't think that require_strong_typing=False is useful anymore - we can always throw an exception when a unannotated method is found.

LEW21 commented 7 years ago

(Of course for methods on the default interface, @interface is not required.)

xZise commented 7 years ago

Okay I'm currently working on implementing your suggestions. One question I have is, what do you mean with @typed_signal? Maybe that is related to your confusion regarding the usage of @signalled, but I don't think that makes sense. The XML generator basically only checks the getter to determine what type is required.

xZise commented 7 years ago

Okay I didn't noticed you had answered in the mean time, so I have actually worked on using decorators and not annotations. I personally prefer that route, as it won't break Python 2.7 (which is still widely used) and I don't think it is that much of a overhead.

As I like your second suggestion, just using one @interface decorator, and as I have already implemented the strongly typed decorators, I could try add annotation support as well to actually look how much of a overhead this would create (and I'm probably going to push it to the repo so you can take a look as well).

LEW21 commented 7 years ago

About typed_signal - objects can send signals, and currently that's being defined with

from pydbus.generic import signal

class X:
   SignalName = signal()

To autogenerate XML for them, we need a typed_signal that takes the types of signal's arguments.

xZise commented 7 years ago

Ah okay you are right. But that might be a bit more complicated as you cannot annotate assignments, so it would be something like this:

class X:
    SignalName = typed_signal(TYPES)

(having to call signal inside typed_signal seems superfluous)

I have applied (probably) all of your suggestions except using annotations, but hopefully I'll have that today. (No guarantee regarding typed_signal though, I don't know yet how to approach it)

LEW21 commented 7 years ago

That's why I haven't suggested typed_signal as a decorator :D I didn't write "@" before its name :D

Probably it should be a class that inherits pydbus.generic.signal, and has a different constructor.

xZise commented 7 years ago

Yeah I noticed that. I just wanted to make sure we are on the same page regarding it not being a decorator (especially if you consider that everything else are basically decorators).

xZise commented 7 years ago

Okay I have now uploaded the changes I did. It currently has also some support for signals but there are still a few questions remaining things. For one you used lowercase names (like signal), while the Python convention is to use CamelCase for class names (like TypedSignal). I followed the Python convention for now. It is similar with the imports which are alphabetically sorted, then by “type” (first import X and then from X import Y) lastly by “source” (first stdlib, then 3rd party and then local packages).

I also haven't added any tests for the annotations yet, as they cannot be used together with Python 2, so they need to be separate files. I actually haven't set up pydbus in Python 3 because the default Python is 2.7. EDIT Actually I can just call python3 😒

Also the TypedSignal cannot use annotations at all and also needs to define the name too. Currently it is all just in one parameter which is directly used in the for loop (so a list of tuples would work). Maybe there are better approaches? It doesn't actually add simple signals, but I just wanted to get this revision out first.

LEW21 commented 7 years ago

(GitHub collapsed one of two TypedSignal-related comments as outdated, you might need to click "Show outdated")

xZise commented 7 years ago

Regarding the self parameter in signal dummy methods: This is actually not as important as the method is (afaik) never bound to the class and even then it would never be called. So it could actually not skip defining self. Now my patch actually uses it because it can reuse the code to extract the argument types, which is automatically discarding the first argument.

AlexejStukov commented 7 years ago

Ok, I know this may be too late, but you could use the new type hints with python2.7 in the same file. All you have to do is move them to a single line comment beneath the function signature (see here). But I don't really know if there is an equivalent for inspect.signature that works on python2.7 or if you would have to write something that handles that.

xZise commented 7 years ago

This patch supports type hints using the Python 3 syntax but there is no such support for Python 2. One major reason is that it would have to actually parse the file itself, because (as far as I can tell) the annotations using comments aren't actually parsed by Python.

poncovka commented 7 years ago

Hi, how does it look with this pull request? We would like to use this approach in anaconda.

poncovka commented 3 years ago

We have implemented our own solution with type hints and we heavily depend on it in our project. This implementation is now part of the dasbus library (https://github.com/rhinstaller/dasbus) that we have started to use instead of pydbus.