Summary

Add local branch information to rbt status

Review Request #7951 — Created Feb. 7, 2016 and submitted April 4, 2016, 10:48 p.m.

Information

Owner

evanhunzinger

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

Modified rbt post to append current local branch to the POST request, modified the rbt status to obtain the saved local branch from the GET request, and altered the formatting of rbt status to show the last local branch that was used to post the review request.

Testing Done

I tested both rbt post and rbt status manually.

`rbt post`

Create branches in both git and bzr repos and do rbt post and ensure that local_branch is added to the review request as extra_data
Create bookmark in hg repo and do rbt post and ensure that local_bookmark is added to the review request as extra_data
Do an rbt post in and hg repo without specifying a bookmark and ensure that no local_bookmark is not added to the review request as extra data
Do an rbt post with an SCM that doesn't have branching/bookmarking capabilities (I used svn) and ensure no unintended extra_data is being added

`rbt status`

Run rbt status in repos that support branching (both git and bzr) and ensure that only the 'Branch' column shows up
Run rbt status in repos that support branching (hg) and ensure that only the 'Bookmark' column shows up
Run rbt status in a repo that don't support branching/bookmarking (I used svn) and ensure that neither 'Branch' nor 'Bookmark' columns show up
Run rbt status --all and ensure that both the 'Branch' and 'Bookmark' columns show up, and also that each branch and bookmark shows up properly associated with the correct review request
Make terminal large enough to fit each record of the rbt status output in one line and ensure that it outputs properly
Shrink terminal small enough to require word wraps for various records in the rbt status output and ensure that it outputs properly

Files

Issues

Description	From	Last Updated
I think we should print a blank string instead of "None"	david	April 1, 2016, 9:48 p.m.
I wouldn't mind other opinions on this, it looks like TextTable will try to limit the size of the table …	EV evanhunzinger	April 2, 2016, 12:30 p.m.
This should be extra_data.local_branch.	brennie	Feb. 8, 2016, 3:07 p.m.
Col: 30 E128 continuation line under-indented for visual indent	reviewbot	Feb. 14, 2016, 11:09 a.m.
Col: 30 W503 line break before binary operator	reviewbot	Feb. 14, 2016, 11:09 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Feb. 14, 2016, 11:09 a.m.
Col: 30 E128 continuation line under-indented for visual indent	reviewbot	Feb. 14, 2016, 11:09 a.m.
Col: 30 W503 line break before binary operator	reviewbot	Feb. 14, 2016, 11:09 a.m.
Col: 80 E501 line too long (121 > 79 characters)	reviewbot	Feb. 14, 2016, 11:09 a.m.
This should just be None, in my opinion.	brennie	Feb. 8, 2016, 3:43 p.m.
This should just be None, in my opinion.	brennie	Feb. 14, 2016, 11:09 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Feb. 14, 2016, 12:27 p.m.
Col: 30 E113 unexpected indentation	reviewbot	Feb. 14, 2016, 12:32 p.m.
Col: 30 E111 indentation is not a multiple of four	reviewbot	Feb. 14, 2016, 12:32 p.m.
Col: 30 E111 indentation is not a multiple of four	reviewbot	Feb. 14, 2016, 12:32 p.m.
This should just be blank, actually. That way, if a user did have an actual None branch, it wouldnt be …	brennie	Feb. 21, 2016, 12:01 p.m.
Col: 80 E501 line too long (99 > 79 characters)	reviewbot	Feb. 21, 2016, 12:14 p.m.
Col: 80 E501 line too long (102 > 79 characters)	reviewbot	Feb. 21, 2016, 12:14 p.m.
Col: 80 E501 line too long (99 > 79 characters)	reviewbot	Feb. 21, 2016, 12:14 p.m.
We're not consistent (especially in old code like this file), but docstrings should be in the imperative mood rather than …	david	March 6, 2016, 11:36 a.m.
This is getting pretty ugly and repetitive. Can we abstract this somehow so we store the table and then print …	david	March 6, 2016, 11:36 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 6, 2016, 11:49 a.m.
I wasn't sure about this solution, should I be utilizing the SCMClient Capabilities object instead?	EV evanhunzinger	March 6, 2016, 12:06 p.m.
Col: 17 W503 line break before binary operator	reviewbot	March 6, 2016, 11:49 a.m.
Col: 17 W503 line break before binary operator	reviewbot	March 6, 2016, 11:49 a.m.
Col: 17 E128 continuation line under-indented for visual indent	reviewbot	March 6, 2016, 11:49 a.m.
Pretty close. Formatting should be: Returns: bytes: A string with ...	david	March 19, 2016, 12:36 p.m.
Same here with the "type"	david	March 19, 2016, 12:36 p.m.
This is a third party import It should be in its own group. Also, you'll have to update setup.py to …	brennie	March 26, 2016, 12:56 p.m.
Hmm. What happens in the case where a user does --all with both git and hg repos? In that case …	david	March 26, 2016, 12:52 p.m.
This doesn't handle the can_bookmark case. Perhaps we should just have if 'branch' in request: ?	david	March 26, 2016, 12:52 p.m.
Dedent this 4 spaces ('local_bookmark' should start on the same column as self.tool.can_bookmark)	david	March 19, 2016, 12:37 p.m.
Dedent this 2 spaces.	david	March 19, 2016, 12:37 p.m.
The len fields are no longer required, right?	david	March 19, 2016, 12:37 p.m.
This is no longer required, right?	david	March 19, 2016, 12:38 p.m.
Missing trailing comma.	brennie	March 31, 2016, 11:29 p.m.
This can be simplified a bit. We don't need to have the not has_* checks in there (we can always …	david	April 1, 2016, 9:47 p.m.
If you reorder this a bit, it becomes simpler: request_stats = { 'status': status, 'summary': 'r/%s - %s' % (request.id, …	david	April 1, 2016, 9:47 p.m.
Col: 68 E502 the backslash is redundant between brackets	reviewbot	April 2, 2016, 12:30 p.m.
Col: 59 E703 statement ends with a semicolon	reviewbot	April 2, 2016, 12:30 p.m.
Col: 17 E128 continuation line under-indented for visual indent	reviewbot	April 2, 2016, 12:30 p.m.
This should be grouped with the other 3rd party imports (the texttable one)	david	April 3, 2016, 12:12 p.m.
Is it possible to skip all this and just pass in the max width to the Texttable constructor?	david	April 3, 2016, 12:12 p.m.
Please keep this is alphabetical order.	david	April 3, 2016, 12:12 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	April 3, 2016, 12:12 p.m.

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py

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

Show all issues

Col: 30
 E128 continuation line under-indented for visual indent

rbtools/commands/status.py (Diff revision 1)
Show all issues
```
Col: 30
 W503 line break before binary operator
```

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

Show all issues

Col: 80
 E501 line too long (80 > 79 characters)

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

Show all issues

Col: 30
 E128 continuation line under-indented for visual indent

rbtools/commands/status.py (Diff revision 1)
Show all issues
```
Col: 30
 W503 line break before binary operator
```

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

Show all issues

Col: 80
 E501 line too long (121 > 79 characters)

rbtools/commands/post.py (Diff revision 1)

Show all issues

This should be extra_data.local_branch.

brennie Feb. 8, 2016, 3:08 p.m.

Actually, I forgot that extra_data__ gets converted to extra_data. in request methods, so this is fine.

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

Show all issues

This should just be None, in my opinion.

brennie Feb. 8, 2016, 3:43 p.m.

RB filed two issues at once. This must be a bug :(

rbtools/commands/status.py (Diff revision 1)
Show all issues
```
This should just be None, in my opinion.
```

Change Summary:

Fixed formatting issues addressed by Review Bot as well as suggested change from Barret.

Diff:

Revision 2 (+26 -6)

Show changes

	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py

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

Show all issues

Col: 80
 E501 line too long (80 > 79 characters)

Change Summary:

Fixed another line length issue.

Diff:

Revision 3 (+26 -6)

Show changes

	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py

rbtools/commands/status.py (Diff revision 3)
Show all issues
```
Col: 30
 E113 unexpected indentation
```

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

Show all issues

Col: 30
 E111 indentation is not a multiple of four

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

Show all issues

Col: 30
 E111 indentation is not a multiple of four

Change Summary:

Revised a silly mistake, more formatting fixes.

Diff:

Revision 4 (+25 -5)

Show changes

	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py

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

Show all issues

This should just be blank, actually. That way, if a user did have an actual None branch, it wouldnt be confusing

Change Summary:

Added bazaar integration, fixed formatting bug with rbt status if the review request summary length was less than the length of the related header, and changed initial value of branch to be empty when fetching the data from the request.

Diff:

Revision 5 (+35 -6)

Show changes

	rbtools/clients/bazaar.py
	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/bazaar.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/bazaar.py

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

Show all issues

Col: 80
 E501 line too long (99 > 79 characters)

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

Show all issues

Col: 80
 E501 line too long (102 > 79 characters)

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

Show all issues

Col: 80
 E501 line too long (99 > 79 characters)

Change Summary:

Did the max() calls seperately to make the code more explicit and also to fit within the line criteria.

Diff:

Revision 6 (+38 -5)

Show changes

	rbtools/clients/bazaar.py
	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/bazaar.py



Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/bazaar.py

rbtools/clients/bazaar.py (Diff revision 6)

Show all issues

We're not consistent (especially in old code like this file), but docstrings should be in the imperative mood rather than the passive ("Return the ..." vs. "Returns ...")

Can you also add the new "Returns:" section as per https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/#returns ?

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

Show all issues

This is getting pretty ugly and repetitive. Can we abstract this somehow so we store the table and then print it out in a more generic way?

brennie March 2, 2016, 12:44 p.m.

What about something like pretty table ?

evanhunzinger March 6, 2016, 11:36 a.m.

I was taking a look at both text table and pretty table, I'll try them out tonight and see how well they work out. It will also clean up the code that I'm working on right now for handling rbt status for SCMClients that don't have branch information since I don't want to output that branch column if that SCM doesn't have branch capabilities.

How do I handle using external python libraries in Review Board? I'm used to using node.JS where you can run npm install ___ --save or modifying the dependencies.json file manually, I'm not familiar with how to do it for this project though.

Note: I forgot to submit this message, left it as a draft by accident!

Change Summary:

Added texttable module to clean up table creation code, modified method comments, and check to see if the SCM is either Git, Mercurial, or Bazaar before drawing the last column.

Diff:

Revision 7 (+71 -14)

Show changes

	rbtools/clients/bazaar.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/clients/mercurial.py
    rbtools/commands/post.py
    rbtools/clients/bazaar.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/clients/mercurial.py
    rbtools/commands/post.py
    rbtools/clients/bazaar.py
    rbtools/clients/git.py

rbtools/commands/post.py (Diff revision 7)

Show all issues

Col: 80
 E501 line too long (80 > 79 characters)

rbtools/commands/status.py (Diff revision 7)
Show all issues
```
Col: 17
 W503 line break before binary operator
```
rbtools/commands/status.py (Diff revision 7)
Show all issues
```
Col: 17
 W503 line break before binary operator
```

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

Show all issues

Col: 17
 E128 continuation line under-indented for visual indent

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

Show all issues

I wasn't sure about this solution, should I be utilizing the SCMClient Capabilities object instead?

david March 6, 2016, 11:42 a.m.

The capabilities object is sent by the server, and represents what's available server-side. We should instead add a new can_something property to SCMClient (there are several there already) and override it in the tools which support it.

EV

evanhunzinger March 6, 2016, 11:51 a.m.
```
OK, I'll give that a go.
```

Change Summary:

Fixed code formatting issues.

Diff:

Revision 8 (+71 -14)

Show changes

	rbtools/clients/bazaar.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/clients/mercurial.py
    rbtools/commands/post.py
    rbtools/clients/bazaar.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/clients/mercurial.py
    rbtools/commands/post.py
    rbtools/clients/bazaar.py
    rbtools/clients/git.py

Testing Done:

Added can_branch and can_bookmark properties to SCMClient for determining capabilities in both status and post commands.

Diff:

Revision 9 (+78 -14)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py

Testing Done:

Added can_branch and can_bookmark properties to SCMClient for determining capabilities in both status and post commands.

rbtools/clients/git.py (Diff revision 9)

Show all issues

Pretty close. Formatting should be:

Returns:
    bytes:
    A string with ...

rbtools/clients/mercurial.py (Diff revision 9)
Show all issues
```
Same here with the "type"
```

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

Show all issues

Hmm. What happens in the case where a user does --all with both git and hg repos? In that case I think we'll always say "Bookmark" even for things that are branches.

Perhaps just always call it "branch"? "local branch"?

Anyone else have a suggestion?

evanhunzinger March 8, 2016, 10:24 p.m.

I didn't know of that ability, does --all still just look at the same directory so it handles the case where the user has two repos in the same directory? That seems to complicate any functionality in RBTools where self.tool is utilized, which SCMClient would be initialized to self.tool?

david March 8, 2016, 10:36 p.m.

Based on my reading of the code (and understanding of what the feature does), it doesn't filter based on the current directory. In that case, the tool and repository are initialized to match the current directory but not actually used. We should probably change it so those don't get initialized if they're not needed.

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

Show all issues

This doesn't handle the can_bookmark case. Perhaps we should just have if 'branch' in request: ?

evanhunzinger March 8, 2016, 10:24 p.m.

You're right, this was a bit of an oversight that worked because I added can_branch to the hg SCMClient which is unrelated but allowing this to work. I'll switch that over.

evanhunzinger March 8, 2016, 11:06 p.m.

I realized that there is a case where there can be a review request from an SCMClient that has branching/bookmarking capabilities (so they have the third header, therefore require three elements in each row). If that request doesn't have a bookmark associated with it, it won't be in the request data from the API call, meaning there won't be a 'branch' property in the request. So how about for this I add the can_bookmark in the if statement and still use the .get() function to return None if it doesn't exist, but will at least append as a value to the row?

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

Show all issues

Dedent this 4 spaces ('local_bookmark' should start on the same column as self.tool.can_bookmark)

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

Show all issues

Dedent this 2 spaces.

evanhunzinger March 8, 2016, 10:24 p.m.

Just out of curiosity, why is this one only 2 spaces and the one above 4 spaces?

evanhunzinger March 8, 2016, 10:49 p.m.

Never mind, I didn't take into consideration the extra characters in elif.

rbtools/commands/status.py (Diff revision 9)
Show all issues
```
The len fields are no longer required, right?
```
1. EV
  
  evanhunzinger March 8, 2016, 10:24 p.m.
  You're right, I'll get rid of those.
rbtools/commands/status.py (Diff revision 9)
Show all issues
```
This is no longer required, right?
```

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

Show all issues

This is a third party import It should be in its own group. Also, you'll have to update setup.py to add it as a dependency.

evanhunzinger March 19, 2016, 12:34 p.m.

Is this just done by appending a package name to the install_requires list?

brennie March 21, 2016, 2:52 p.m.
```
Yes
```

Change Summary:

Removed unused code and fixed code formatting.

Diff:

Revision 10 (+78 -16)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py

Change Summary:

Removed dependency on self.tool and based table creation based off of the contents of 'local_branch' and 'local_bookmark' in extra_data instead.

Diff:

Revision 11 (+95 -16)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py

Change Summary:

Added 'texttable' as a dependency in setup.py.

Diff:

Revision 12 (+96 -16)

Show changes

	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py
    setup.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py
    setup.py

Can you attach some sample output of the table as a screenshot or text?

Also, this is probably worth removing the [WIP] tag on.

evanhunzinger March 31, 2016, 11:33 p.m.

I uploaded three images from three different scenarios. Let me know if you want the decorators to look different or anything!

setup.py (Diff revision 12)
Show all issues
```
Missing trailing comma.
```

Change Summary:

Added a trailing comma to the third party install list.

Summary:

-	[WIP] Add local branch information to rbt status
+	Add local branch information to rbt status

Diff:

Revision 13 (+96 -16)

Show changes

	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py
    setup.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py
    setup.py

This is looking pretty good. You said you uploaded three examples of output but I don't see them.

EV

evanhunzinger April 1, 2016, 10:54 a.m.
```
I left it as a draft again, sorry!
```

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

Show all issues

This can be simplified a bit. We don't need to have the not has_* checks in there (we can always just set to True if it's present). We can also skip the early break because it doesn't buy very much.

for request in request_stats:
    if 'branch' in request:
        has_branches = True

    if 'bookmark' in request:
        has_bookmarks = True

evanhunzinger April 1, 2016, 11:04 a.m.

OK, my idea was that the not has_* checks could save some time in the and conditional, but you're right that the savings are minimal at the expense of less consise code.

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

Show all issues

If you reorder this a bit, it becomes simpler:

request_stats = {
    'status': status,
    'summary': 'r/%s - %s' % (request.id, request.summary),
}

if 'local_branch' in request.extra_data:
    request_stats['branch'] = \
        request.extra_data['local_branch']

if 'local_bookmark' in request.extra_data:
    request_stats['bookmark'] = \
        request.extra_data['local_bookmark']

Added Files:

Sample output from all repos (Revision 1)

Show all issues

I wouldn't mind other opinions on this, it looks like TextTable will try to limit the size of the table (and therefore columns) if you don't manually set size constraints on the column, which leads to crunched up columns like this in the --all case. I was thinking of determining column size at run time based off the largest values, but then that could mean really wide tables which probably won't print well if someone is running a small console.

david April 1, 2016, 11:33 a.m.

Can we use https://pypi.python.org/pypi/backports.shutil_get_terminal_size to determine the width of the console and use that for the max-width of the table?

Sample output from all repos (Revision 1)

Show all issues

I think we should print a blank string instead of "None"

Change Summary:

Cleaned up code and changed None output to empty strings.

Diff:

Revision 14 (+86 -17)

Show changes

	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py
    setup.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py
    setup.py

Change Summary:

Added adaptive sizing of table based off terminal size.

Diff:

Revision 15 (+114 -13)

Show changes

	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py
    setup.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py
    setup.py

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

Show all issues

Col: 68
 E502 the backslash is redundant between brackets

rbtools/commands/status.py (Diff revision 15)
Show all issues
```
Col: 59
 E703 statement ends with a semicolon
```

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

Show all issues

Col: 17
 E128 continuation line under-indented for visual indent

Change Summary:

Fixed formatting issues.

Diff:

Revision 16 (+114 -13)

Show changes

	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/post.py
	rbtools/commands/status.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py
    setup.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py
    setup.py

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

Show all issues

This should be grouped with the other 3rd party imports (the texttable one)

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

Show all issues

Is it possible to skip all this and just pass in the max width to the Texttable constructor?

evanhunzinger April 2, 2016, 9:58 p.m.

Do you mean to just solve for the max once when I put the values in col_widths? (ex. combining line 54 and 61)

david April 2, 2016, 10:34 p.m.

Based on my reading of the texttable code, if you pass in the number of columns to the constructor [ table = tt.Texttable(get_terminal_size().columns) ], it should do its own calculation to try to size the columns nicely.

evanhunzinger April 3, 2016, 11:52 a.m.

Ah, you're right! I should have read the documentation more closely, would have saved me some time.

setup.py (Diff revision 16)

Show all issues

Please keep this is alphabetical order.

Change Summary:

Removed logic to set column widths manually since texttable can do it. Also rearranged import and alphabetized installs in setup script.

Diff:

Revision 17 (+85 -17)

Show changes

	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/post.py
	rbtools/commands/status.py

Added Files:

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py
    setup.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py
    setup.py

rbtools/commands/status.py (Diff revision 17)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

Fixed line length issue and removed images that shouldn't have been associated with the update.

Diff:

Revision 18 (+87 -17)

Show changes

	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/post.py
	rbtools/commands/status.py

Removed Files:

Tool: Pyflakes
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py
    setup.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/status.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/bazaar.py
    setup.py

This looks good to me. Can you add some testing information?

Testing Done:

	+	I tested both `rbt post` and `rbt status` manually.
	+
	+	`rbt post`
	+
	+	Create branches in both git and bzr repos and do `rbt post` and ensure that `local_branch` is added to the review request as `extra_data`
	+	Create bookmark in hg repo and do `rbt post` and ensure that `local_bookmark` is added to the review request as `extra_data`
	+	Do an `rbt post` in and hg repo without specifying a bookmark and ensure that no `local_bookmark` is not added to the review request as extra data
	+	Do an `rbt post` with an SCM that doesn't have branching/bookmarking capabilities (I used svn) and ensure no unintended `extra_data` is being added
	+
	+	`rbt status`
	+
	+	Run `rbt status` in repos that support branching (both git and bzr) and ensure that only the 'Branch' column shows up
	+	Run `rbt status` in repos that support branching (hg) and ensure that only the 'Bookmark' column shows up
	+	Run `rbt status` in a repo that don't support branching/bookmarking (I used svn) and ensure that neither 'Branch' nor 'Bookmark' columns show up
	+	Run `rbt status --all` and ensure that both the 'Branch' and 'Bookmark' columns show up, and also that each branch and bookmark shows up properly associated with the correct review request
	+	Make terminal large enough to fit each record of the `rbt status` output in one line and ensure that it outputs properly
	+	Shrink terminal small enough to require word wraps for various records in the `rbt status` output and ensure that it outputs properly

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (63b91e6)