BIND 10 #1541: script for listing obsolete (fully merged) branches

[BIND 10 Development]

#1541: script for listing obsolete (fully merged) branches
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:
                   Priority:  low    |  Sprint-20120320
                  Component:  build  |            Resolution:
  system                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  Core
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => jinmei


Comment:

 Replying to [comment:5 jinmei]:
 > Overall, it seems to do what it intends to do.  I see some minor
 > points (mainly style related ones), but excessive purism is probably
 > not very productive especially for this type of helper tools.  I'll
 > make some comments, but let's not spend too much time on it.  Maybe
 > just after one more round of cleanup we can simply merge it and close
 > the ticket.
 Ok.

 > - for parsing options, I'd suggesting using the OptionParser module.
 >   see, for example, bind10_src.py.in how to use it.
 Updated code to use OptionParser.

 > - this doesn't seem to be very clean:
 > {{{#!python
 > #   Uncomment this to print out also merged branches
 > #    branch_print(branch_list, False, True, False)
 > }}}
 >   I'd control the behavior via a command line option, especially when
 >   we already have some options.
 It was a leftover, no longer needed. This behavior is controlled via
 command-line options. Removed it.

 > - I'd like to see some more comments about what each function is
 >   expected to do, and some more inline comments.  There's no single
 >   right answer this request, but for example, calling a subprocess
 >   would generally need an explanation..  The intent of filtering some
 >   outputs in branch_list_get is not very obvious (to me).
 Added more comments with explanation what is happening.

 > - Some style(-like) matters:
 >
 >   - "Branch" doesn't seem to have to be a class.  A tuple would be
 >     sufficient, although this may be a matter of taste
 If time permits, I would like to expand the code and Branch class will be
 more complicated. Defining it as a class would ease further development.

 >   - I'd add a space between '=' here (at least the spacing policy
 >     isn't consistent):
 > {{{#!python
 >     MERGED=1
 >     NOTMERGED=2
 > }}}
 Done.

 >   - For "not assigned" type of placeholder value, we normally use
 >     'None':
 > {{{#!python
 >     name = ""
 >     status = NOTMERGED
 >     last_commit = ""
 > }}}
 Done.

 >   - we don't need a semilcolon at EOL (I removed them)
 Thanks.

 >   - we don't need parentheses for 'if' conditions, e.g:
 > {{{#!python
 >         if (len(diff) == 0):
 > }}}
 >     same for 'return'.
 Done as suggested. By the way, why are we not trying to use similar coding
 standards for C++ and python if language syntax allows it?

 >   - this should probably be just removed:
 > {{{#!python
 > #        return (out)
 > }}}
 Removed.

 Thanks for your review.

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