[Bug 32353] improve <annotation> support

[bugzilla-daemon at freedesktop.org]

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

--- Comment #2 from Alex Merry <fdo-bugs at randomguy3.me.uk> 2010-12-13 16:51:04 PST ---
(In reply to comment #1)
> > @@ -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.

Oh, yes, I picked that up later, but forgot to go back and change this.

> 
> 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.

The logic is: presence of a tp:deprecated element (ie: self.deprecated is not
None) sets self.is_deprecated to true unconditionally.  If that element doesn't
exist (self.deprecated is None), self.is_deprecated is set based on the
existance and value of the annotation (if the annotation is absent, the ==
'true' obviously results in False).

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

OK.

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

OK.


> 
> +    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.

Sure.  In general, I'd prefer this way, but I was following how it worked
already.  I've moved the emits_changed stuff as well:
http://gitorious.org/randomguy3/telepathy-spec/commit/e9127a411d337754a7c0d1eb52c70e5740bb605f

> 
> > +    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.

Following existing style :-)

Should I change the ACCESS_READ and ACCESS_WRITE alignments as well, or leave
it all as is?

> 
> > +        # 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.

The assumption behind that decision was that bindings would take care of this
automatically.  I still think it was the wrong decision, though, and I may
raise it on the D-Bus list.

> 
> > +    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/>?

Yeah, mind-blip.

http://gitorious.org/randomguy3/telepathy-spec/commit/102070535317543d158ea3b212e1525da45d6fde
and
http://gitorious.org/randomguy3/telepathy-spec/commit/e9127a411d337754a7c0d1eb52c70e5740bb605f

-- 
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.