Add extra output when checking review request statuses.

Review Request #7640 — Created Sept. 18, 2015 and submitted

jwu
RBTools
master
a387117...
rbtools

Originally, rbt status would only show the review request summary and the review request id.

The change will give the status of each review request, showing how many 'Open Issues' there are or how many 'Ship It!' there are. If there are no open issues, and the review request hasn't been submitted, it shows the 'Ship It!' count

Below is an example output:

(rbenv)vagrant@precise64:/src/reviewboard-2.0.x/reviewboard$ rbt status
===========================================================================
Status            Review Request
===========================================================================
Open Issues (2)   r/5 - Integer a amet Aliquam pharetra dui
Draft             r/6 - Quisque adipiscing tortor,  amet velit orci
Ship It! (1)      r/3 - adipiscing congue ac fames non,  posuere
Pending           r/7 - ipsum id. mi dui,  Integer laoreet
Pending           r/4 - magna In aliquet tincidunt egestas Suspendisse

(rbenv)vagrant@precise64:/src/reviewboard-2.0.x/reviewboard$

Ran rbt status to get the above output.

Ran rbt tests.

Description From Last Updated

For the arguments that get passed to the max method, I think that it would be more Pythonic if we ...

AD adriano

Needs a docstring. Docstrings should be of the form """Single line summary. Multi-line description. """

brenniebrennie

Since you are passing a generator, you only need one set of parenthesis.

brenniebrennie

Style nitpick: blank line between these.

brenniebrennie

Col: 35 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 33 E703 statement ends with a semicolon

reviewbotreviewbot

This should be written in the imperative mood (something like "Print review request statuses in a table").

daviddavid

Python str has a nifty ljust method, which can simplify this quite a bit: white_space = max_status_length + TAB_SIZE for ...

daviddavid

Docstrings should have a summary that fits entirely on one line and then a longer description (if necessary). They should ...

daviddavid

These should use single quotes.

daviddavid

We should only say "Ship It" if there are one or more "Ship It" reviews. By default we should probably ...

daviddavid

It might be better to calculate this in the tabulate method, or to calculate all the lengths here. It's also ...

daviddavid

These could be formatted more nicely by being a bit less verbose and wrapping differently: max_status_length = max(d['status_len'] for d ...

daviddavid

Instead of using end='', let's just print both of these at the same time: print('%s%s' % ('Status'.ljust(white_space), 'Review Request'))

daviddavid

Can you document the arguments to this function? We document our args as follows: """... Args: arg_name (arg_type): Description of ...

brenniebrennie

Can you comment on what these do?

brenniebrennie

Can you format this like: status = request['status'].ljust(white_space) print('%s%s' % (status, request['summary'])

brenniebrennie

You don't need the quotes here. print() should work.

brenniebrennie

Can you document the args and return of the function as follows: ``` """... Args: arg_name (arg_type): Description of Arg ...

brenniebrennie

"Return" and not "Get". "Get" implies that we are querying for informatino.

brenniebrennie

Blank line between these. Also, data is really generic.

brenniebrennie

data is very generic. Can we call this something else? Perhaps request_stats?

brenniebrennie

Can you put the comment directly before each one, e.g. # The number of spaces between a request status ...

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/status.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/status.py
    
    
  2. 
      
AD
  1. 
      
  2. rbtools/commands/status.py (Diff revision 1)
     
     
     
     
     

    For the arguments that get passed to the max method, I think that it would be more Pythonic if we replaced the list comprehensions with generator expressions (that is, we can simply drop the outermost square brackets). That way, we aren't allocating memory for an intermediate list; instead, each integer is being generated on the fly. You might find this article useful: Code Like a Pythonista: Idiomatic Python

    1. Thanks! Completely forgot about generators.

  3. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/status.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/status.py
    
    
  2. 
      
brennie
  1. I like the output, but it may be too much if you have a lot of review requests. Currently, they only take up one line each so its easy to get a summary at a glance. I think it would be better if it were formatted like:

    =============================================
    Status           Review Request
    =============================================
    Draft            r/1234 - Some Review Request
    Ship it!         r/1235 - Some Review Request
    Open Issues (4)  r/1236 - Some Review Request
    
  2. rbtools/commands/status.py (Diff revision 2)
     
     

    Needs a docstring. Docstrings should be of the form

    """Single line summary.
    
    Multi-line description.
    """
    
  3. rbtools/commands/status.py (Diff revision 2)
     
     
     

    Since you are passing a generator, you only need one set of parenthesis.

  4. rbtools/commands/status.py (Diff revision 2)
     
     
     

    Style nitpick: blank line between these.

  5. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/status.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/status.py
    
    
  2. rbtools/commands/status.py (Diff revision 3)
     
     
    Col: 35
     E127 continuation line over-indented for visual indent
    
  3. rbtools/commands/status.py (Diff revision 3)
     
     
    Col: 33
     E703 statement ends with a semicolon
    
  4. 
      
JW
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/status.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/status.py
    
    
  2. 
      
david
  1. 
      
  2. rbtools/commands/status.py (Diff revision 4)
     
     

    This should be written in the imperative mood (something like "Print review request statuses in a table").

  3. rbtools/commands/status.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     

    Python str has a nifty ljust method, which can simplify this quite a bit:

    white_space = max_status_length + TAB_SIZE
    
    for row in data:
        print '%s%s' % (row[0].ljust(white_space), row[1])
    

    You can use it above for the header, too.

  4. rbtools/commands/status.py (Diff revision 4)
     
     
     

    Docstrings should have a summary that fits entirely on one line and then a longer description (if necessary). They should also be written in the imperative mood (So "Get the current status" rather than "Gets ...").

  5. rbtools/commands/status.py (Diff revision 4)
     
     
     
     

    These should use single quotes.

  6. rbtools/commands/status.py (Diff revision 4)
     
     
     

    We should only say "Ship It" if there are one or more "Ship It" reviews. By default we should probably just say something like "Pending" or "Open".

  7. rbtools/commands/status.py (Diff revision 4)
     
     

    It might be better to calculate this in the tabulate method, or to calculate all the lengths here.

    It's also not great to stuff a bunch of stuff into a list like this. It means that people reading the tabulate method have to then go read this method to figure out what's in the list. Perhaps use a dictionary instead?

    detail = 'r/%s - %s' % (request.id, request.summary)
    review_data.append({
        'status': status,
        'detail': detail,
        'status_len': len(status),
        'detail_len': len(detail),
        'row_len': len(status) + len(detail),
    })
    
  8. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/status.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/status.py
    
    
  2. 
      
david
  1. 
      
  2. rbtools/commands/status.py (Diff revision 5)
     
     
     
     
     
     

    These could be formatted more nicely by being a bit less verbose and wrapping differently:

    max_status_length = max(d['status_len'] for d in data)
    max_row_length = (max(d['summary_len'] for d in data) +
                      PADDING + TAB_SIZE + max_status_length)
    
  3. rbtools/commands/status.py (Diff revision 5)
     
     
     

    Instead of using end='', let's just print both of these at the same time:

    print('%s%s' % ('Status'.ljust(white_space), 'Review Request'))
    
  4. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/status.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/status.py
    
    
  2. 
      
david
  1. Can you update the example output in the description?

    Also, in your summary, the plural of "status" is "statuses" (possesive would use status')

    1. Updated. Thanks for all the speedy responses :)

  2. 
      
JW
brennie
  1. 
      
  2. rbtools/commands/status.py (Diff revision 6)
     
     

    Can you document the arguments to this function?

    We document our args as follows:

    """...
    
    Args:
        arg_name (arg_type):
            Description of arg.
    """
    

    You don't need to document self.

  3. rbtools/commands/status.py (Diff revision 6)
     
     
     

    Can you comment on what these do?

  4. rbtools/commands/status.py (Diff revision 6)
     
     
     

    Can you format this like:

                status = request['status'].ljust(white_space)
                print('%s%s' % (status, request['summary'])
    
  5. rbtools/commands/status.py (Diff revision 6)
     
     

    You don't need the quotes here. print() should work.

  6. rbtools/commands/status.py (Diff revision 6)
     
     
     

    Can you document the args and return of the function as follows:

    ```
    """...

    Args:
    arg_name (arg_type):
    Description of Arg

    Returns:
    return_type: Description of return value
    """

  7. rbtools/commands/status.py (Diff revision 6)
     
     

    "Return" and not "Get". "Get" implies that we are querying for informatino.

  8. rbtools/commands/status.py (Diff revision 6)
     
     
     

    Blank line between these.

    Also, data is really generic.

  9. rbtools/commands/status.py (Diff revision 6)
     
     

    data is very generic. Can we call this something else? Perhaps request_stats?

  10. 
      
JW
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/status.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/status.py
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/status.py (Diff revision 7)
     
     
     
     
     
     

    Can you put the comment directly before each one, e.g.

    # The number of spaces between a request status ...
    TAB_SIZE = 3
    
    # THe number of ...
    PADDING = 5
    

    These should also probably be class constants, e.g.

    class Status(Command):
        # ...
    
        # The number of spaces...
        TAB_SIZE = 3
    

    and you can refer to them with either Status.TAB_SIZE or self.TAB_SIZE.

  3. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/status.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/status.py
    
    
  2. 
      
JW
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (56bc998)
Loading...