MPRIS uses <annotation>, so their version of the telepathy-spec toolchain has some improvements in that direction. http://gitorious.org/randomguy3/telepathy-spec/commit/82a80f89e04c4ba208bdd8bf24e01b0286dd124d
> @@ -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.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.