`rbt land` command for landing changes in upstream

Review Request #6509 — Created Oct. 26, 2014 and submitted

Information

RBTools
master
2f08592...

Reviewers

rbt land command for landing changes in upstream
See https://reviewboard.hackpad.com/rbt-land-YrgGuhTuseU
for detailed description of the command.

What works:

  • Merging/rebasing a branch with/on destination branch.
  • Adding a commit message based on the review request.
  • Pushing to upstream
  • A wrapper around rbt patch in order to allow landing
    others' changes.
  • Successfully merged and rebased a feature branch to/on master branch.

  • Successfully patched a review request from another user and landed it
    on upstream.

  • Tested with:

    • empty repo
    • unclean working directory
    • unapproved review request
    • same target and destination branches
    • incorrect target/destination branch
    • incorrect upstream repository

and got appropriate error messages, as expected.

Description From Last Updated

'APIError' imported but unused

reviewbotreviewbot

'execute' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

'json' imported but unused

reviewbotreviewbot

local variable 'remote' is assigned to but never used

reviewbotreviewbot

'APIError' imported but unused

reviewbotreviewbot

'InvalidRevisionSpecError' imported but unused

reviewbotreviewbot

'execute' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

'json' imported but unused

reviewbotreviewbot

'json' imported but unused

reviewbotreviewbot

'json' imported but unused

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

'json' imported but unused

reviewbotreviewbot

Let's use "squash" instead of "flatten". Also, this function and push_upstream need to have NotImplemented stubs in SCMClient.

chipx86chipx86

Blank line between these.

chipx86chipx86

Say you do this on 'master', and your commit is already there. Doing a pull is going to pull down …

chipx86chipx86

Missing a trailing period.

chipx86chipx86

This doesn't really describe what the command does. We should have a lot more detail about what this is and …

chipx86chipx86

The """ should be on the next lin.

chipx86chipx86

Merging shouldn't squash. Or are you saying it'd be the inverse?

chipx86chipx86

Can this be done now?

chipx86chipx86

Just from the common ones, right? How about doing that as part of this change?

chipx86chipx86

'json' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

undefined name 'branch'

reviewbotreviewbot

undefined name 'branch'

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

'json' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

'json' imported but unused

reviewbotreviewbot

Col: 69 W291 trailing whitespace

reviewbotreviewbot

'json' imported but unused

reviewbotreviewbot

'json' imported but unused

reviewbotreviewbot

Use a double quote and put the branch name in single quotes, e.g. raise MergeError("Could not checkout branch '%s' ...) …

brenniebrennie

Quote branchname in single quotes.

brenniebrennie

"to the remote repository"

chipx86chipx86

.reviewboardrc shoudn't be required. It hsould be possible to specify on the command line.

chipx86chipx86

I prefer full sentences that sound more natural, like "A review request ID is required."

chipx86chipx86

This change I think predates the discussion on this, but "BRANCH" can't be used for any kind of real determinations …

chipx86chipx86

This may not be very clear when the user hits it and doesn't realize that they chose the same branch. …

chipx86chipx86

We can't depend on this attribute existing on the review request. It's new in 2.0. You can check with hasattr(). …

chipx86chipx86

"The specified review request ..." This should also include the review_request.approval_failure text.

chipx86chipx86

This should now be MergeError as e. Same in all other cases.

chipx86chipx86

How about "... has landed on <branch>"

chipx86chipx86

Since we're moving to Py2.5+, you'll want to use the new print function. At the top of your file: from …

brenniebrennie

This was changed in https://reviews.reviewboard.org/r/6578/ to use __future__.print_function. You should pull master and rebase your changes off of it. Your …

brenniebrennie

I had to read the code and think through it to realize what this is doing and why it's there. …

chipx86chipx86

There's a way to simplify this and make the function in general a lot more useful. Instead of hard-coding all …

chipx86chipx86

'json' imported but unused

reviewbotreviewbot

Col: 36 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 36 E113 unexpected indentation

reviewbotreviewbot

Col: 17 E113 unexpected indentation

reviewbotreviewbot

Col: 17 E112 expected an indented block

reviewbotreviewbot

Col: 25 W601 .has_key() is deprecated, use 'in'

reviewbotreviewbot

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

reviewbotreviewbot

"are" considered optional.

chipx86chipx86

"an upstream repository."

chipx86chipx86

"the patching step."

chipx86chipx86

Instead of hard-coding here, it's best to identify what's missing in SCMClients that this function needs to support. We can …

chipx86chipx86

This needs to raise a CommandError for the output to be properly shown. Otherwise it'll appear as a crash. Also, …

chipx86chipx86

I just pushed a change that adds this. See what commands/stamp.py and commands/post.py do now.

chipx86chipx86

"Review Board". Can you say: "... an old version (pre-2.0)" that ..."

chipx86chipx86

"support the `approved` field."

chipx86chipx86

Blank line between these.

chipx86chipx86

If we have approved and approval_failure, we have issue_open_count. They were introduced in the same release.

chipx86chipx86

The ending paren should be on the self.options.squash line.

chipx86chipx86

We don't put parens around individual conditions.

chipx86chipx86

Same here.

chipx86chipx86

This looks left over from something.

chipx86chipx86

You can probably make this one line by doing: approval_failure = \ 'The review request ...'

chipx86chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        setup.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        setup.py
        rbtools/clients/git.py
    
    
  2. rbtools/commands/land.py (Diff revision 1)
     
     
    Show all issues
     'APIError' imported but unused
    
  3. rbtools/commands/land.py (Diff revision 1)
     
     
    Show all issues
     'execute' imported but unused
    
  4. rbtools/commands/land.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. rbtools/commands/land.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W391 blank line at end of file
    
  6. setup.py (Diff revision 1)
     
     
    Show all issues
     'json' imported but unused
    
  7. 
      
AS
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        setup.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        setup.py
        rbtools/clients/git.py
    
    
  2. rbtools/clients/git.py (Diff revision 2)
     
     
    Show all issues
     local variable 'remote' is assigned to but never used
    
  3. rbtools/commands/land.py (Diff revision 2)
     
     
    Show all issues
     'APIError' imported but unused
    
  4. rbtools/commands/land.py (Diff revision 2)
     
     
    Show all issues
     'InvalidRevisionSpecError' imported but unused
    
  5. rbtools/commands/land.py (Diff revision 2)
     
     
    Show all issues
     'execute' imported but unused
    
  6. rbtools/commands/land.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  7. rbtools/commands/land.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  8. rbtools/commands/land.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  9. rbtools/commands/land.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W391 blank line at end of file
    
  10. setup.py (Diff revision 2)
     
     
    Show all issues
     'json' imported but unused
    
  11. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        setup.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        setup.py
        rbtools/clients/git.py
    
    
  2. setup.py (Diff revision 3)
     
     
    Show all issues
     'json' imported but unused
    
  3. 
      
AS
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        setup.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        setup.py
        rbtools/clients/git.py
    
    
  2. setup.py (Diff revision 4)
     
     
    Show all issues
     'json' imported but unused
    
  3. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        setup.py
        rbtools/utils/process.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        setup.py
        rbtools/utils/process.py
        rbtools/clients/git.py
    
    
  2. rbtools/commands/land.py (Diff revision 5)
     
     
    Show all issues
    Col: 1
     W391 blank line at end of file
    
  3. setup.py (Diff revision 5)
     
     
    Show all issues
     'json' imported but unused
    
  4. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        setup.py
        rbtools/utils/process.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        setup.py
        rbtools/utils/process.py
        rbtools/clients/git.py
    
    
  2. setup.py (Diff revision 6)
     
     
    Show all issues
     'json' imported but unused
    
  3. 
      
chipx86
  1. 
      
  2. rbtools/clients/git.py (Diff revision 6)
     
     
    Show all issues

    Let's use "squash" instead of "flatten".

    Also, this function and push_upstream need to have NotImplemented stubs in SCMClient.

  3. rbtools/clients/git.py (Diff revision 6)
     
     
     
    Show all issues

    Blank line between these.

  4. rbtools/clients/git.py (Diff revision 6)
     
     
     
     
    Show all issues

    Say you do this on 'master', and your commit is already there. Doing a pull is going to pull down new content and merge it in, creating a bit of an annoying merge graph, before pushing.

    What you probably want here is 'git pull --rebase'.

    Also, pull and push will, depending on settings/version of git, pull/push all branches. I'd be explicit with taking a branch name and passing it in this function.

  5. rbtools/clients/git.py (Diff revision 6)
     
     
    Show all issues

    Missing a trailing period.

  6. rbtools/commands/land.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    This doesn't really describe what the command does. We should have a lot more detail about what this is and why/how it's used.

  7. rbtools/commands/land.py (Diff revision 6)
     
     
    Show all issues

    The """ should be on the next lin.

    1. In Patch, """ is placed as in this one. Should I change it too, or should I keep this?

  8. rbtools/commands/land.py (Diff revision 6)
     
     
    Show all issues

    Merging shouldn't squash. Or are you saying it'd be the inverse?

    1. Yes, since this is more about squashing the history vs. keeping it, rather than merging vs. rebasing. So, --squash would squash everything into one commit, while default would preserve the history.

  9. rbtools/commands/land.py (Diff revision 6)
     
     
     
     
    Show all issues

    Can this be done now?

  10. rbtools/commands/land.py (Diff revision 6)
     
     
     
    Show all issues

    Just from the common ones, right?

    How about doing that as part of this change?

  11. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
  2. rbtools/clients/git.py (Diff revision 7)
     
     
    Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  3. rbtools/clients/git.py (Diff revision 7)
     
     
    Show all issues
     undefined name 'branch'
    
  4. rbtools/clients/git.py (Diff revision 7)
     
     
    Show all issues
     undefined name 'branch'
    
  5. rbtools/utils/commands.py (Diff revision 7)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  6. rbtools/utils/commands.py (Diff revision 7)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  7. setup.py (Diff revision 7)
     
     
    Show all issues
     'json' imported but unused
    
  8. 
      
AS
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
  2. rbtools/clients/git.py (Diff revision 8)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. setup.py (Diff revision 8)
     
     
    Show all issues
     'json' imported but unused
    
  4. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
  2. rbtools/clients/git.py (Diff revision 9)
     
     
    Show all issues
    Col: 69
     W291 trailing whitespace
    
  3. setup.py (Diff revision 9)
     
     
    Show all issues
     'json' imported but unused
    
  4. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
  2. setup.py (Diff revision 10)
     
     
    Show all issues
     'json' imported but unused
    
  3. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
  2. setup.py (Diff revision 11)
     
     
    Show all issues
     'json' imported but unused
    
  3. 
      
chipx86
  1. 
      
  2. rbtools/commands/land.py (Diff revision 11)
     
     
    Show all issues

    "to the remote repository"

  3. rbtools/commands/land.py (Diff revision 11)
     
     
    Show all issues

    .reviewboardrc shoudn't be required. It hsould be possible to specify on the command line.

  4. rbtools/commands/land.py (Diff revision 11)
     
     
    Show all issues

    I prefer full sentences that sound more natural, like "A review request ID is required."

  5. rbtools/commands/land.py (Diff revision 11)
     
     
    Show all issues

    This change I think predates the discussion on this, but "BRANCH" can't be used for any kind of real determinations like this. That option purely handles the visual display of a branch, and can contain any text.

  6. rbtools/commands/land.py (Diff revision 11)
     
     
    Show all issues

    This may not be very clear when the user hits it and doesn't realize that they chose the same branch. How about:

    "The local branch cannot be merged onto itself. Try a different local branch or destination branch."

  7. rbtools/commands/land.py (Diff revision 11)
     
     
    Show all issues

    We can't depend on this attribute existing on the review request. It's new in 2.0.

    You can check with hasattr().

    If it doesn't exist, we can check review_request.ship_it_count.

  8. rbtools/commands/land.py (Diff revision 11)
     
     
     
    Show all issues

    "The specified review request ..."

    This should also include the review_request.approval_failure text.

    1. I changed this line to only print approval_failure text, since it clearly explains the problem.

  9. rbtools/commands/land.py (Diff revision 11)
     
     
    Show all issues

    This should now be MergeError as e.

    Same in all other cases.

  10. rbtools/commands/land.py (Diff revision 11)
     
     
    Show all issues

    How about "... has landed on <branch>"

  11. rbtools/utils/commands.py (Diff revision 11)
     
     
    Show all issues

    I had to read the code and think through it to realize what this is doing and why it's there.

    How about calling this build_rbtools_cmd_argv, and having the docs be clear that this is used to take common options passed on the command line or in .reviewboardrc and convert them back into a set of arguments for calling another RBTools command?

  12. rbtools/utils/commands.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    There's a way to simplify this and make the function in general a lot more useful.

    Instead of hard-coding all these here, this function can take a dictionary (defaulting to a predefiend dictionary constant in this module) and build the command lines from that. It can do this with:

    ```python
    def build_rbtools_cmd_argv(options, options_map=DEFAULT_OPTIONS_MAP):
    argv = []

    for option_key, arg_name in six.iteritems(options_map):
        option_value = getattr(options, option_key, None)
    
        if option_value is True:
            argv.append(arg_name)
        elif option_value not in (False, None):
            argv.extend([arg_name, option_value])
    
    return argv
    
  13. 
      
brennie
  1. 
      
  2. rbtools/clients/git.py (Diff revision 11)
     
     
    Show all issues

    Use a double quote and put the branch name in single quotes, e.g.

    raise MergeError("Could not checkout branch '%s' ...)
    

    It also might read better if the error is on the same line, seperated by a colon instead of newlines.

    Same below.

    1. Added single quotes as you suggested. But newlines are there to seperate error message from SCM command output.

  3. rbtools/clients/git.py (Diff revision 11)
     
     
    Show all issues

    Quote branchname in single quotes.

  4. rbtools/commands/land.py (Diff revision 11)
     
     
    Show all issues

    Since we're moving to Py2.5+, you'll want to use the new print function. At the top of your file:

    from __future__ import print_function
    

    and then here you call print as a function with parentheses: print('Review request %s ...' % request_id)

    Likewise elsewhere you use print.

  5. rbtools/commands/patch.py (Diff revision 11)
     
     
    Show all issues

    This was changed in https://reviews.reviewboard.org/r/6578/ to use __future__.print_function.

    You should pull master and rebase your changes off of it. Your patch may not cleanly apply, otherwise.

  6. 
      
AS
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
  2. rbtools/commands/land.py (Diff revision 12)
     
     
    Show all issues
    Col: 36
     E111 indentation is not a multiple of four
    
  3. rbtools/commands/land.py (Diff revision 12)
     
     
    Show all issues
    Col: 36
     E113 unexpected indentation
    
  4. rbtools/commands/land.py (Diff revision 12)
     
     
    Show all issues
    Col: 17
     E113 unexpected indentation
    
  5. rbtools/commands/land.py (Diff revision 12)
     
     
    Show all issues
    Col: 17
     E112 expected an indented block
    
  6. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
  2. 
      
AS
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/__init__.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/__init__.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
  2. rbtools/commands/land.py (Diff revision 14)
     
     
    Show all issues
    Col: 25
     W601 .has_key() is deprecated, use 'in'
    
  3. 
      
AS
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/__init__.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/__init__.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
  2. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/__init__.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/__init__.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
  2. rbtools/commands/land.py (Diff revision 16)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/__init__.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/__init__.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
  2. 
      
chipx86
  1. Just a few things left. Some small wording changes, and a couple things we can now implement based on a recent commit.

    Looking great! Can't WAIT to land this :)

  2. rbtools/commands/__init__.py (Diff revision 17)
     
     
    Show all issues

    "are" considered optional.

  3. rbtools/commands/land.py (Diff revision 17)
     
     
    Show all issues

    "an upstream repository."

  4. rbtools/commands/land.py (Diff revision 17)
     
     
    Show all issues

    "the patching step."

  5. rbtools/commands/land.py (Diff revision 17)
     
     
     
     
    Show all issues

    Instead of hard-coding here, it's best to identify what's missing in SCMClients that this function needs to support. We can then add a can_<something> boolean attribute to SCMClient, defaulting to False, and add an equivalent to the supported SCMClients, set to True.

    This would then check for those attributes instead. See the new rbt stamp for another case where this is done.

    I imagine we'd want can_merge and can_push_upstream.

    This way, other SCMClients will get support for this when those functions are implemented.

  6. rbtools/commands/land.py (Diff revision 17)
     
     
    Show all issues

    I just pushed a change that adds this. See what commands/stamp.py and commands/post.py do now.

    1. I will do this as a follow up, in a seperate review request.

  7. rbtools/commands/land.py (Diff revision 17)
     
     
    Show all issues

    "Review Board".

    Can you say: "... an old version (pre-2.0)" that ..."

  8. rbtools/commands/land.py (Diff revision 17)
     
     
     
    Show all issues

    Blank line between these.

  9. rbtools/commands/land.py (Diff revision 17)
     
     
     
     
    Show all issues

    If we have approved and approval_failure, we have issue_open_count. They were introduced in the same release.

  10. rbtools/commands/land.py (Diff revision 17)
     
     
     
    Show all issues

    The ending paren should be on the self.options.squash line.

  11. rbtools/utils/commands.py (Diff revision 17)
     
     
    Show all issues

    We don't put parens around individual conditions.

  12. rbtools/utils/commands.py (Diff revision 17)
     
     
    Show all issues

    Same here.

  13. rbtools/utils/commands.py (Diff revision 17)
     
     
    Show all issues

    This looks left over from something.

  14. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/__init__.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/__init__.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
  2. 
      
chipx86
  1. 
      
  2. rbtools/commands/land.py (Diff revisions 17 - 18)
     
     
     
    Show all issues

    This needs to raise a CommandError for the output to be properly shown. Otherwise it'll appear as a crash.

    Also, can you put the space after the %s instead of on the next line, and the % self.tool.name on the next line? Like:

    raise CommandError('This command ... %s '
                       'repositories.'
                       % self.tool.name)
    
    1. Actually, even better would be:

      raise CommandError(
          'This command ... repositories.'
          % self.tool.name)
      

      If that fits.

  3. rbtools/commands/land.py (Diff revisions 17 - 18)
     
     
    Show all issues

    "support the `approved` field."

  4. rbtools/commands/land.py (Diff revision 18)
     
     
     
    Show all issues

    You can probably make this one line by doing:

    approval_failure = \
        'The review request ...'
    
  5. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/__init__.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/__init__.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
  2. 
      
AS
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/__init__.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
        rbtools/utils/commands.py
        rbtools/commands/__init__.py
        rbtools/commands/patch.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        setup.py
        rbtools/clients/errors.py
    
    Ignored Files:
        AUTHORS
    
    
  2. 
      
AS
Review request changed
Status:
Completed
Change Summary:
Pushed to master (e11deb8)