Bug 32353

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

(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

(In reply to comment #2)
> 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).

Hmm, I see how it works now. I think it'd be clearer if you moved "self.is_deprecated = True" to be an "else" clause (i.e. self.deprecated is not None), rather than setting it to True and then immediately changing it to False in the (common!) non-deprecated case.

(In reply to comment #2)
> > > +    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 :-)

Fair enough... you're welcome to PEP8ify either/both, or leave both as-is.

Other than that, this looks good.

(In reply to comment #3)
> Other than that, this looks good.

Fixed those issues:
http://gitorious.org/randomguy3/telepathy-spec/commit/ed2f926721535558f86c1b01ba8d541dddba7d5f

I don't have push access to the telepathy repo - could you merge for me?

Repo URL is git://gitorious.org/randomguy3/telepathy-spec.git, BTW.

Fixed in git for 0.21.8, thanks for your contributions!

(In reply to comment #6)
> Fixed in git for 0.21.8, thanks for your contributions!

There's always one more thing, isn't there?

http://gitorious.org/randomguy3/telepathy-spec/commit/149af482110a953b1a0feedffb075f8c4fe33239

This uses is_deprecated in the templates, rather than deprecated.  It has absolutely no effect on the telepathy docs themselves, but if the D-Bus annotation for deprecation is used but the telepathy annotation isn't, this will make sure everything is displayed correctly.

Merged, thanks.