Bug 30649 - Give warning for unknown interface elements
Summary: Give warning for unknown interface elements
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-spec (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Jonny Lamb
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/jo...
Whiteboard: review+
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-06 03:33 UTC by Jonny Lamb
Modified: 2010-10-06 06:21 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Jonny Lamb 2010-10-06 03:33:34 UTC
Call.Stream.Interface.Media had a tp:method for ages which of course wasn't picked up and didn't do anything. This patch will warn with unknown elements in interfaces.

It's my first specparser.py patch, and it looks kind of ugly. Neat thoughts on how to do it more nicer are appreciated.
Comment 1 Simon McVittie 2010-10-06 04:23:31 UTC
Short term, the patch looks fine and it's better than not having it.

Longer term, one way to do it would be to have some sort of schema. I hear the cool kids all use RELAX NG these days, but W3C XML Schema would also work. DTDs aren't really suitable since they're not namespace-aware.
Comment 2 Will Thompson 2010-10-06 04:38:52 UTC
Great idea.

How about:

  expected_elts = set(
      (XMLNS_DBUS, 'method'),
      ...,
      (XMLNS_TP, 'property'),
      ...)
  surprising_children = [
      x for x in dom.childNodes
        if (x.namespaceURI, x.localName) not in expected_elts
      ]

  for x in surprising_children:
      ...

I'm not sure if it's any clearer.

I think it might be clearer to put the check elsewhere. You could change the stuff from 'build lists of methods, etc...' onwards in Interface.__init__() to iterate self, and have a despatch table for each supported element name, crying about any unrecognised ones. (The code currently uses getElementsByTagNameNS() which searches all descendants, not just immediate children, but I'm a little suspicious of that anyway. ☺)

+                dom.getElementsByTagNameNS(XMLNS_TP, 'causes-havoc') +

causes-havoc is an attribute, I think.
Comment 3 Jonny Lamb 2010-10-06 05:40:54 UTC
(In reply to comment #2)
> How about:
[snip]
> I'm not sure if it's any clearer.

I think it's a bit nicer, yes, and has less repetition. I think your second suggestion might be nice but for now I'm going to avoid lots of big changes if that's okay? :-)

> causes-havoc is an attribute, I think.

Yes good point, removed.
Comment 4 Will Thompson 2010-10-06 05:46:02 UTC
PEP 8 says:

> The preferred place to break around a binary operator is
> *after* the operator, not before it.

So this part of your comprehension is bad and wrong:

+            if isinstance(x, xml.dom.minidom.Element)
+            and (x.namespaceURI, x.localName) not in expected

It should be:

+            if isinstance(x, xml.dom.minidom.Element) and
+               (x.namespaceURI, x.localName) not in expected

(lining up the second condition with the first one).

Fair point about the second suggestion.
Comment 5 Jonny Lamb 2010-10-06 06:05:06 UTC
(In reply to comment #4)
> PEP 8 says:

PEP 8 is balls.

Updated anyway.
Comment 6 Will Thompson 2010-10-06 06:14:02 UTC
Yeah!!!, this is the right architecture for software!!!!!!!
Comment 7 Jonny Lamb 2010-10-06 06:21:02 UTC
kthxbye


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.