[Bug 32353] improve <annotation> support

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=32353

--- Comment #1 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-12-13 11:49:43 PST ---
> @@ -142,6 +162,9 @@ class Base(object):
>          self.docstring = getOnlyChildByName(dom, XMLNS_TP, 'docstring')
>          self.added = getOnlyChildByName(dom, XMLNS_TP, 'added')
>          self.deprecated = getOnlyChildByName(dom, XMLNS_TP, 'deprecated')
> +        self.is_deprecated = True
> +        if self.deprecated == None:
> +            self.is_deprecated = getAnnotationByName(dom, 'org.freedesktop.DBus.Deprecated') == 'true'

"if self.deprecated is None" is considered better Python style - None is
unusual in this respect.

This logic seems to make self.is_deprecated true in all cases: I think you
want something like

    self.is_deprecated = not self.deprecated

before setting it based on the presence/absence of annotations.

I'd prefer () around "getAnnotation...(...) == 'true'" to make the precedence
obvious, or an explicit if/else.

+        self.no_reply = getAnnotationByName(dom,
'org.freedesktop.DBus.Method.NoReply') == 'true'

Same here.

+    def get_no_reply(self):
+        if self.no_reply:
+            return '<div class="annotation no-reply">' \
+                   'The caller should not expect a reply when calling this
method.</div>'
+        else:
+            return ''

It'd be slightly inconsistent with the other (pseudo-)annotations, but could
this just be done in the template rather than having a method that returns a
HTML blob? specparser.py is meant to just produce a data structure, although
in practice it ends up with quite a lot of HTML presentation embedded in it.
This case looks simple enough to just do it in the template.

> +    EMITS_CHANGED_UNKNOWN     = 0
> +    EMITS_CHANGED_NONE        = 1
> +    EMITS_CHANGED_UPDATES     = 2
> +    EMITS_CHANGED_INVALIDATES = 3

Nitpicking: PEP 8 recommends not aligning like that, but just having "x = y"
with one space on each side.

> +        # According to the D-Bus specification, EmitsChangedSignal defaults
> +        # to true, but - realistically - this cannot be assumed for old specs

Wow, I hadn't spotted that - if we believe that, then introducing
PropertiesChanged made every existing service instantly buggy! You are correct
to treat absence as "no idea", IMO.

> +    def get_emits_changed(self):
> +        if self.emits_changed == self.EMITS_CHANGED_UPDATES:
> +            return '<div class="annotation emits-changed emits-changed-updates">' \
> +                   'When this property changes, the ' \
> +                   '<literal>org.freedesktop.DBus.Properties.PropertiesChanged</literal> ' \
> +                   'signal is emitted with the new value.</div>';

<literal/> isn't HTML. You could use <code/>?

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.