BIND 10 #905: TSIG: complete python library update

[BIND 10 Development]

#905: TSIG: complete python library update
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110517
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  DNSPacket API                      |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  2.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:8 stephen]:

 Again, thanks for the review!

 > Reviewing differences between commit
 e93f054a2d0cf1c21d00112ccaa93d5d2432a677 (where branch for #893 was merged
 immediately after creating the branch for #905) and commit
 9eddc07cd32918f3b8e9ebd114d9c8f8f39a359b (latest commit).

 Ah, yes, that's right.  The merge commit was expected to be ignored.
 I forgot to mention that in the review request.

 '''src/lib/dns/python/tsigerror_python.cc'''[[BR]]
 > Would help if there was a little more documentation on the class; for
 example, it would be helpful to highlight the different constructors (one
 with an integer and the copy constructor).

 (We don't have/need a python binding for the copy constructor, but
 anyway) you mean documentation for the python lib embedded in the C++
 code, right?  If so, fair enough...and I've decided to use this
 opportunity to address a long standing issue: reducing the overhead of
 providing the python doc.  Most of the doc should be able to be a copy
 of the corresponding C++ doc, in theory it should be possible to share
 a single source of doc for both C++ and python.  I've written a python
 script that parses XML output from doxygen and adjusts it for the
 python library.  There are still some non trivial points that cannot
 automatically converted, so we still need to edit the output by hand,
 but it should now be much easier to provide quite consistent set of
 document for both C++ and python.

 I've added a new file, tsigerror_python_inc.cc, which contains the
 half-automated with minimally edited documentation.
 tsigerror_python.cc includes it.

 I plan to cleanup the script and include it (after review) in our
 source tree, but that will be a separate task.

 > '''Other'''
 >
 > > This approach requires careful consideration of inclusion ordering,
 which is becoming a difficult task as the binding gets bigger.
 > Why not just include #ifdef sentinels in the .cc files and include them
 in one another as required?  Providing there are no loops, the inclusion
 ordering won't matter.

 Hmm, I didn't think about that possibility, but I suspect it's not
 always that simple due to possible circular dependency (tsig could
 have included message although I didn't choose that option, and
 message would need to include tsig, etc).  Unless we at least separate
 header files (where we declare minimum things, some of which would be
 just forward declaration) and implementation files, this approach
 won't work.  Having circular dependency itself often indicates a
 design issue and (if we have it) may need to be addressed anyway, but
 it would also be nicer if the python binding works regardless of the
 existence of circular dependency.

 If we separated headers and implementations, it might be a matter of
 style whether or not we combine the implementation files into a single
 .cc by #include.  Although I personally think it's more natural to
 keep different implementation files different translation units, I do
 not necessarily object that if you strongly prefer combining them.

 > > It's also error prone because we sometimes use 'using namespace' in
 .cc files, but with this approach this means doing so before including
 some other header files, which is generally considered a bad practice.
 > Maybe, but as there are only a few different namespaces being "used" (I
 found six, including "std"), why not just put them at the head of the
 pydnspp.cc and be done with it.
 >
 > An alternative would be to put the appropriate "using" declaration
 within some of the longer functions (so limiting the scope to just that
 function) and use the namespace in type declarations elsewhere.

 (I'm afraid I may not fully understand the first approach - it simply
 seems to have the same problem, but in any case) such workaround may
 work, but even so, the problem is that it's quite easy for us to
 forget such unusual convention and break it.  Again, while I don't
 necessarily disagree with keeping combining them, I don't see much
 benefit to do so despite the fragile workaround.

 But, to be clear, if that's your strong preference, I don't
 necessarily object and am okay with moving forward with it as long as
 we separate header files and avoid 'using namespace' before including
 header files.

-- 
Ticket URL: <http://bind10.isc.org/ticket/905#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development