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 naokikambe):

 Replying to [comment:11 jinmei]:
 Thank you for reviewing. But I'm not sure whether you are a real reviewer
 for this ticket. So I left it '!UnAssigned'. If this is wrong, please
 assign yourself.

 > '''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.
 I updated the docstring at
 {{{
 5fdb1fa [2298] update the docstring of item_name_list()
 }}}

 > - 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 += '/'
 > }}}
 After I considered that well, I found it was ok to assume it to be a
 string. For the above comments, I revised the code at
 {{{
 3d0e3ec [2298] It was ok to assume the argument identifier to be a string
 }}}

 > - 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.
 According to your comment, I revised the code at
 {{{
 aa38883 [2298] use '%d' in formatting an integer instead of '%s'
 }}}

 > '''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.

 I added comments for the tests at:
 {{{
 c0b0bf7 [2298] add comments in test_item_name_list()
 }}}

 BTW, I changed the way to sort the key names in element which is in the
 first argument.
 Before revising, returned value was sorted at the end of the method.
 However, idexes were also sorted as strings. For example, regarding a
 10-index list, ``['a[0]', 'a[1]', .. 'a[10]']`` are wrongly soterd to
 ``['a[0]', 'a[1]', 'a[10]', .. 'a[9]']``. So I fixed that at
 {{{
 bd42224 [2298] add a test for sorting
 f3c3c9b [2298] in case of dict change the way to sort by key name
 }}}

 Regards,

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