Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1541 closed enhancement (fixed)

script for listing obsolete (fully merged) branches

Reported by: tomek Owned by: tomek
Priority: low Milestone: Sprint-20120320
Component: build system Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 1.25 Internal?: no

Description

According to our git guidelines, fully merged branch should be deleted. Many people (including author of this ticket) forgets to do it and number of unneeded branches keeps growing. We need a script that will list branches that are merged.

It should print out information about likely owner of the branch to ease removal process

It should print out data in CSV format, so its output could be processed further.

Subtickets

Change History (11)

comment:1 Changed 8 years ago by tomek

  • Owner changed from UnAssigned to tomek
  • Status changed from new to accepted

comment:2 Changed 8 years ago by tomek

  • Owner changed from tomek to UnAssigned
  • Status changed from accepted to reviewing

This script works with python2.7 and 3.1.

Ready for review.

Last edited 8 years ago by tomek (previous) (diff)

comment:3 Changed 8 years ago by jreed

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:4 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120320

comment:5 follow-up: Changed 8 years ago by 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.

  • for parsing options, I'd suggesting using the OptionParser? module. see, for example, bind10_src.py.in how to use it.
  • this doesn't seem to be very clean:
    #   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.
  • 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).
  • 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
  • I'd add a space between '=' here (at least the spacing policy isn't consistent):
        MERGED=1
        NOTMERGED=2
    
  • For "not assigned" type of placeholder value, we normally use 'None':
        name = ""
        status = NOTMERGED
        last_commit = ""
    
  • we don't need a semilcolon at EOL (I removed them)
  • we don't need parentheses for 'if' conditions, e.g:
            if (len(diff) == 0):
    
    same for 'return'.
  • this should probably be just removed:
    #        return (out)
    

comment:6 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to tomek

comment:7 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by tomek

  • Owner changed from tomek to jinmei

Replying to 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:
    #   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 become more complicated over time. Defining it as a class would ease further development.

  • I'd add a space between '=' here (at least the spacing policy isn't consistent):
        MERGED=1
        NOTMERGED=2
    

Done.

  • For "not assigned" type of placeholder value, we normally use 'None':
        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:
            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:
    #        return (out)
    

Removed.

Thanks for your review.

Last edited 8 years ago by tomek (previous) (diff)

comment:8 in reply to: ↑ 7 Changed 8 years ago by jinmei

Replying to tomek:

I made a few more minor editorial fixes. With these it looks okay.
Please merge.

  • we don't need parentheses for 'if' conditions, e.g:
            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?

Because (in my understanding) that's what the vast majority of python
developers do. Again in my understanding the basic principle is that
if there's a clear majority in preferring a particular style in that
language's community, we follow it; that would make our code more
natural for them, and that would make third party contribution easier.

comment:9 Changed 8 years ago by jinmei

  • Owner changed from jinmei to tomek

comment:10 Changed 8 years ago by tomek

  • Resolution set to fixed
  • Status changed from reviewing to closed

Branch merged.

comment:11 Changed 8 years ago by jinmei

  • Total Hours changed from 0 to 1.25
Note: See TracTickets for help on using tickets.