Add mercurial hook for posting review requests automatically

Review Request #7824 — Created Dec. 23, 2015 and discarded

Information

RBTools
master

Reviewers

A Mercurial hook to post to Review Board on push to a central server.

The hook only allows the push if all commits have been approved in a
review requests, otherwise it rejects it and creates a review request
for the (new) commits. This is similar to "rbt post", but

1. does not require the user to install RBTools locally
2. makes sure any changes pushed to the central server have been
   reviewed and approved
3. makes links of all references to tickets/bugs/issues, which
   rbt post doesn't do.
4. automatically finds the right review request to update if there
   are any new commits, based the commit ID and a date/author hash.
   This does not require the user to confirm anything, which rbt post
   (often) requires. This also allows the hook to recognize
   rebased/amended changesets, because the date/author hash
   is unchanged.

Patch is based on work of Halvor Lund!

Replacing this: https://reviews.reviewboard.org/r/7100/

We use this commit hook since february 2015 as a gatekeeper like gerrit codereview.

Description From Last Updated

We've been slowly transitioning to Google Python-style docstrings. Can you write this as: Run the hook. Args: node (unicode): The …

daviddavid

Can you use the Args: format as described above?

daviddavid

Can we wrap this so each condition is on its own line? You can also combine the second two: if …

daviddavid

Args:

daviddavid

r before s.

daviddavid

ids is already a list here.

daviddavid

We put full-package imports before from ... imports.

daviddavid

Process return codes should be int, not bool.

daviddavid

Please reformat as: revisions = { 'base': rev1, 'tip': rev2, 'parent_base': parent, }

daviddavid

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 23 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 25 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

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

reviewbotreviewbot

Col: 5 E122 continuation line missing indentation or outdented

reviewbotreviewbot

Col: 22 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 24 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 5 E122 continuation line missing indentation or outdented

reviewbotreviewbot

Col: 1 E122 continuation line missing indentation or outdented

reviewbotreviewbot

Col: 15 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 17 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 20 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 22 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 15 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 17 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 11 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 13 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 21 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 23 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 15 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 17 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 26 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 28 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 18 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 20 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 19 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 21 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

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

reviewbotreviewbot

It's a little weird to call this LOGGER, since it's not a constant. How about logger?

daviddavid

Can you surround the whole conditional in parentheses instead of using the continuation character?

daviddavid

These should be at the top of the file.

daviddavid

Can we swap the order of these?

daviddavid

This should be grouped with the other stdlib imports.

daviddavid

Call this "logger" (since it's not really a constant)

daviddavid

Put this import at the top of the file.

daviddavid

Rather than implementing a new execute method, we should use rbtools.utils.process.execute

daviddavid

This should use the approval API. See https://www.reviewboard.org/docs/manual/2.5/webapi/2.0/resources/review-request/#fields

daviddavid

Can you wrap the whole conditional in parens instead of using the continuation character? Please also put the "and" on …

daviddavid

Why doesn't this use rbtools.utils.users.get_authenticated_session ?

daviddavid

Put this import at the top of the file.

daviddavid
david
  1. 
      
  2. contrib/tools/mercurial_push.py (Diff revision 1)
     
     
    Show all issues

    We've been slowly transitioning to Google Python-style docstrings. Can you write this as:

    Run the hook.
    
    Args:
        node (unicode): The hex node ID of the first changeset.
    
  3. contrib/tools/mercurial_push.py (Diff revision 1)
     
     
     
     
    Show all issues

    Can you use the Args: format as described above?

  4. contrib/tools/mercurial_push.py (Diff revision 1)
     
     
     
     
    Show all issues

    Can we wrap this so each condition is on its own line? You can also combine the second two:

    if (revreq.branch == branch and
        'diff_hash' in revreq.extra_data.get('diff_hash', '')):
    
    1. Are you sure? I changed it to this:

      if (revreq.branch == branch and
          diff_hash == revreq.extra_data.get('diff_hash'):
      
    2. You're right. I was confusing this with other code I was reading in another tab (that used in).

  5. contrib/tools/mercurial_push.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    Args:

  6. rbtools/hooks/common.py (Diff revision 1)
     
     
     
    Show all issues

    r before s.

  7. rbtools/hooks/common.py (Diff revision 1)
     
     
    Show all issues

    ids is already a list here.

  8. rbtools/hooks/mercurial.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    We put full-package imports before from ... imports.

  9. rbtools/hooks/mercurial.py (Diff revision 1)
     
     
     
    Show all issues

    Process return codes should be int, not bool.

  10. rbtools/hooks/mercurial.py (Diff revision 1)
     
     
     
     
    Show all issues

    Please reformat as:

    revisions = {
        'base': rev1,
        'tip': rev2,
        'parent_base': parent,
    }
    
  11. 
      
misery
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
    WARNING: Number of comments exceeded maximum, showing 30 of 69.
    
    Tool: Pyflakes
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
  2. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 13
     E128 continuation line under-indented for visual indent
    
  3. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 9
     E128 continuation line under-indented for visual indent
    
  4. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 13
     E128 continuation line under-indented for visual indent
    
  5. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 13
     E128 continuation line under-indented for visual indent
    
  6. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 23
     E251 unexpected spaces around keyword / parameter equals
    
  7. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 25
     E251 unexpected spaces around keyword / parameter equals
    
  8. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 5
     E128 continuation line under-indented for visual indent
    
  9. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 5
     E122 continuation line missing indentation or outdented
    
  10. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 22
     E251 unexpected spaces around keyword / parameter equals
    
  11. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 24
     E251 unexpected spaces around keyword / parameter equals
    
  12. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 5
     E122 continuation line missing indentation or outdented
    
  13. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     E122 continuation line missing indentation or outdented
    
  14. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 15
     E251 unexpected spaces around keyword / parameter equals
    
  15. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 17
     E251 unexpected spaces around keyword / parameter equals
    
  16. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 20
     E251 unexpected spaces around keyword / parameter equals
    
  17. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 22
     E251 unexpected spaces around keyword / parameter equals
    
  18. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 15
     E251 unexpected spaces around keyword / parameter equals
    
  19. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 17
     E251 unexpected spaces around keyword / parameter equals
    
  20. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 11
     E251 unexpected spaces around keyword / parameter equals
    
  21. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 13
     E251 unexpected spaces around keyword / parameter equals
    
  22. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 21
     E251 unexpected spaces around keyword / parameter equals
    
  23. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 23
     E251 unexpected spaces around keyword / parameter equals
    
  24. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 15
     E251 unexpected spaces around keyword / parameter equals
    
  25. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 17
     E251 unexpected spaces around keyword / parameter equals
    
  26. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 26
     E251 unexpected spaces around keyword / parameter equals
    
  27. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 28
     E251 unexpected spaces around keyword / parameter equals
    
  28. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 18
     E251 unexpected spaces around keyword / parameter equals
    
  29. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 20
     E251 unexpected spaces around keyword / parameter equals
    
  30. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 19
     E251 unexpected spaces around keyword / parameter equals
    
  31. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Show all issues
    Col: 21
     E251 unexpected spaces around keyword / parameter equals
    
  32. 
      
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
  2. 
      
HA
  1. Thanks for taking the time to submit this again. What are the main differences from my original submission?

    1. I just fixed the issues in your review request. :-)
      I added a new request because I cannot update your request. I will fix these issues here, too. Hopefully we get this merged!

  2. 
      
misery
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
  2. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
  2. rbtools/hooks/mercurial.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  3. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
  2. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
  2. 
      
HA
  1. Ship It!
  2. 
      
david
  1. 
      
  2. contrib/tools/mercurial_push.py (Diff revision 6)
     
     
    Show all issues

    It's a little weird to call this LOGGER, since it's not a constant. How about logger?

  3. contrib/tools/mercurial_push.py (Diff revision 6)
     
     
     
    Show all issues

    Can you surround the whole conditional in parentheses instead of using the continuation character?

  4. contrib/tools/mercurial_push.py (Diff revision 6)
     
     
     
    Show all issues

    These should be at the top of the file.

  5. rbtools/hooks/mercurial.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    Can we swap the order of these?

  6. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
  2. 
      
david
  1. Sorry for the long delay on this.

  2. rbtools/hooks/mercurial.py (Diff revision 7)
     
     
    Show all issues

    This should be grouped with the other stdlib imports.

  3. rbtools/hooks/mercurial.py (Diff revision 7)
     
     
    Show all issues

    Call this "logger" (since it's not really a constant)

  4. rbtools/hooks/mercurial.py (Diff revision 7)
     
     
    Show all issues

    Put this import at the top of the file.

  5. rbtools/hooks/mercurial.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
    Show all issues

    Rather than implementing a new execute method, we should use rbtools.utils.process.execute

  6. rbtools/hooks/mercurial.py (Diff revision 7)
     
     
     
     
    Show all issues

    This should use the approval API. See https://www.reviewboard.org/docs/manual/2.5/webapi/2.0/resources/review-request/#fields

  7. rbtools/hooks/mercurial.py (Diff revision 7)
     
     
     
    Show all issues

    Can you wrap the whole conditional in parens instead of using the continuation character? Please also put the "and" on the previous line.

  8. rbtools/hooks/mercurial.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Why doesn't this use rbtools.utils.users.get_authenticated_session ?

  9. rbtools/hooks/mercurial.py (Diff revision 7)
     
     
    Show all issues

    Put this import at the top of the file.

  10. 
      
misery
Review request changed
Status:
Discarded
Change Summary:

This is superseded by a complete refactoring: https://reviews.reviewboard.org/r/8554/