openSUSE / dbxincluder

Transclusions for DocBook with XInclude 1.1
https://opensuse.github.io/dbxincluder
GNU General Public License v3.0
2 stars 3 forks source link

Small code improvements #15

Closed tomschr closed 8 years ago

tomschr commented 8 years ago

Enough for today. ;-)

Vogtinator commented 8 years ago

If something is not implemented, I would raise the NotImplementedError exception (see line 117)

According to the python documentation (https://docs.python.org/3/library/exceptions.html#concrete-exceptions), that's not now NotImplementedError should be used. As it's only a single occurence, I'll keep it as DBXIException for now.

Vogtinator commented 8 years ago

Forgot to comment on that:

Collect all namespaces and save it, for example, in a separate file (for example, core.py). Avoids any nasty typos. ;) Include it where necessary.

The dependency hierarchy is dbxincluder.init > dbxincluder.docbook > dbxincluder.xinclude so this isn't needed IMO as every dependency (except utils) is only used once.

tomschr commented 8 years ago

The dependency hierarchy ...

IMHO this hasn't anything to do with dependency hierarchy. :)

For example, in src/dbxincluder/xinclude.py you have six occurances of http://www.w3.org/XML/1998/namespace and two for http://www.w3.org/2001/XInclude/local-attributes.

I think that code cries to refactor that part and use constants to collect all namespaces. If it is in a separate file or in the xinclude.py itself is not really relevant for me. However, duplicate strings should be avoided. Good chances for typos! ;-)

For example, you could start with an NS dict in xinclude.py:


NS=dict(xml='http://www.w3.org/XML/1998/namespace',
   local="http://www.w3.org/2001/XInclude/local-attributes",
   xi="http://www.w3.org/2001/XInclude",
)

With the help of etree.QName you can build your curly-braces-notation automatically:

# using the above dict:
xmlid = etree.QName(NS['xml'], 'id')

Here are some more usecases:

>>> xmlid = etree.QName(NS['xml'], 'id')
>>> xmlid.text
'{http://www.w3.org/XML/1998/namespace}id'
>>> xmlid.namespace
'http://www.w3.org/XML/1998/namespace'
>>> xmlid.localname
'id'
Vogtinator commented 8 years ago

Oh, that's what you meant by namespaces... I thought about namespaces as python includes.

etree.QName looks definitely useful.

tomschr commented 8 years ago

Oh, that's what you meant by namespaces... I thought about namespaces as python includes.

Oh, I'm sorry, I had better say XML namespaces. :grin:

Vogtinator commented 8 years ago

Fixed!