BIND 10 #2298: Xfrout/zones and XML stats

[BIND 10 Development]

#2298: Xfrout/zones  and XML stats
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  UnAssigned
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20121106
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  statistics                         |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 I'm not still sure if I can reasonably review this branch, but since
 no one has picked up, I'll give it a try...

 == commits `56da3a8^..17fb05b` ==

 '''stats_httpd.py'''

 - it's difficult to understand what this function does just from the
   description.  What's "list of items"? (and what are "items"?)  What
   does traverse mean?  By which it's "sorted"?  Also some examples may
   help.  Some in-code comments also help.  I'd like to see description
   about possible exceptions, too.

 - why do we need to call str() if identifier is of string?
 {{{#!python
     if identifier and len(str(identifier)) > 0:
         ident = str(identifier)
         ret.append(ident)
 }}}
 - if we know identifier is a string can't we skip the 'and len(...'?
 {{{#!python
     if identifier and len(str(identifier)) > 0:
 }}}
 - further, since we replace ident with identifier if the latter is not
   empty, can't we simply do this?
 {{{#!python
     ident = identifier
     if ident:
         ret.append(ident)
 }}}
 - why use len() if ident is a string?
 {{{#!python
         if len(ident) > 0:
 }}}
 - this seems to be redundant
 {{{#!python
             ident = '%s/' % ident
 }}}
   I'd do it:
 {{{#!python
             ident += '/'
 }}}
 - is it okay not to consider the case where ident is empty here?
 {{{#!python
             idstr = '%s[%s]' % (ident, i)
 }}}
   also, while minor, it seems to me more natural if we use '%d' for
   the second '%s' because i is an integer.

 '''stats_httpd_test.py'''

 It's not really clear specifically which point each assertXXX tries to
 test.  Is this a set of necessary and sufficient tests? (i.e., isn't
 there an uncovered scenario?  or isn't there a redundant case?).  And,
 in any case, I'd like to see brief comments on the purpose of the test
 for each (major) case.

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