Adding pre-post hook to .reviewboardrc

Review Request #7066 — Created March 16, 2015 and discarded

Information

RBTools
master

Reviewers

This feature is to add pre-post hook to run before posting the request. This patch demonstrate how we can do format checking before posting the review so that we can make edit locally to fix formatting issues locally. The user can use --no-pre-post-hook to turn off the hook if neccessary.

Test with "pyflakes . && nosetests" command specified in rbtools/.reviewboardrc and check if the command is run.
Check if the pre-post-hook is turned off if the --no-pre-post-hook flag is on.

Description From Last Updated

PEP8? I'm not sure if we want to set thi in the global .reviewboardrc.

brenniebrennie

We use single quotes.

brenniebrennie

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

reviewbotreviewbot

Using the shell would save a lot of headaches and make the pre-post hooks more effective and powerful in my …

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

This will not work in the case of quoted substrings.

brenniebrennie

You can use rbtools.utils.console.confirm instead of rolling your own confirmation logic.

brenniebrennie

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 9 E122 continuation line missing indentation or outdented

reviewbotreviewbot

I don't think shell=True is a good idea. These explain it well https://docs.python.org/2/library/subprocess.html#frequently-used-arguments http://stackoverflow.com/questions/3172470/actual-meaning-of-shell-true-in-subprocess

TI tienv

Use single quotes instead of double quotes. And shouldn't is_continue be continue?

VT VTL-Developer

you can probably just do: if not confirm()

TI tienv

I don't know if we want to force this on our users.

brenniebrennie

Missing trailing comma.

brenniebrennie

Missing trailing comma.

brenniebrennie

This should be formatted as: if ('PRE_POST_HOOK' in self.config and not self.options.no_pre_post_hook): # ...

brenniebrennie

Should the result of the influence being able to post ?

brenniebrennie

Single quotes.

brenniebrennie

Col: 48 E502 the backslash is redundant between brackets

reviewbotreviewbot

You removed the newline from the end of this line. Can you revert this change please?

brenniebrennie

Can we just import subprocess and use subprocess.Popen and subprocess.PIPE ?

brenniebrennie

This can be simplified to use an and. See previous comment re status codes vs output.

brenniebrennie

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

reviewbotreviewbot

Add added_in field to indicate the version of RBTools where this option was introduced.

gmyersgmyers

I don't think this change was intended?

gmyersgmyers

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

reviewbotreviewbot

should we use stdout and stderror here so the user can see messages?

brenniebrennie

su before sy

daviddavid

This looks like debug output?

daviddavid

Can we print out a message saying "X command returned an error" before asking for confirmation?

daviddavid

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

reviewbotreviewbot

Col: 31 W503 line break before binary operator

reviewbotreviewbot

Col: 61 W291 trailing whitespace

reviewbotreviewbot

Needs a trailing comma?

SU Sunxperous

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

reviewbotreviewbot

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

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
  2. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  4. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  5. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  6. 
      
brennie
  1. <p>Shouldn't the results of the pre-post hooks determine if the post happens? You usually don't want to be posting when</p>

    1. ...when the tests fail?

      Sorry I apparently got sidetracked and missed finishing the comment.

  2. .reviewboardrc (Diff revision 1)
     
     

    PEP8?

    I'm not sure if we want to set thi in the global .reviewboardrc.

    1. Can you clarify this comment? I thought that what we discussed before.

    2. Pyflakes doesn't call the pep8 tool. We use both pep8 and pyflakes.

      We also ignore some pyflakes warnings (like blank line between class and docstring).

      I'm also not sure if we want to enforce this on our users, but that decision isn't up to me. You should ask Christrian or David. If this has been discussed then feel free to drop, but this should at least call pep8, too

    3. I mean pep8 and pyflakes if I wasn't clear, sorry.

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

    We use single quotes.

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

    Using the shell would save a lot of headaches and make the pre-post hooks more effective and powerful in my opinion.

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

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these.

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

    This will not work in the case of quoted substrings.

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

    You can use rbtools.utils.console.confirm instead of rolling your own confirmation logic.

  10. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
  2. rbtools/commands/post.py (Diff revision 2)
     
     
    Col: 9
     E122 continuation line missing indentation or outdented
    
  3. 
      
VI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
  2. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
  2. 
      
VT
  1. 
      
  2. rbtools/commands/post.py (Diff revision 4)
     
     
     
     
     
     

    Use single quotes instead of double quotes.

    And shouldn't is_continue be continue?

    1. Sorry, continue is a reserved keyword, you can use synonyms like progress or any other words.

  3. 
      
TI
  1. 
      
  2. rbtools/commands/post.py (Diff revision 4)
     
     
     
     
     
     

    I don't think shell=True is a good idea. These explain it well

    https://docs.python.org/2/library/subprocess.html#frequently-used-arguments

    http://stackoverflow.com/questions/3172470/actual-meaning-of-shell-true-in-subprocess

    1. This comment is already addressed by Barrett. We saracfice some security issues for simplicity.

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

    you can probably just do:

    if not confirm()

  4. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
  2. 
      
brennie
  1. 
      
  2. .reviewboardrc (Diff revision 5)
     
     

    I don't know if we want to force this on our users.

  3. rbtools/commands/post.py (Diff revision 5)
     
     

    Missing trailing comma.

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

    Missing trailing comma.

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

    This should be formatted as:

        if ('PRE_POST_HOOK' in self.config and
            not self.options.no_pre_post_hook):
            # ...
    
  6. rbtools/commands/post.py (Diff revision 5)
     
     

    Should the result of the influence being able to post ?

    1. I don't understand this comment. If the PRE_POST_HOOK command result in failure, the subprocess just output the error out but the user still be prompt with 'Do you want to post your review (Yes/No)?'. If they say Yes, then the post command proceed. Is this what you're asking?

    2. What I mean is: should we only be prompting if the subprocess call fails?

    3. I think the meaning if "failure" for this case is vague. For example, the flake8 commands failes meaning that the output is non empty while the nosetests command fails meaning that the error is non empty. So I will capture the stdout and stderr, and promt if they are non empty.

    4. I think it would be more worth-while using the return code the executed command. Having a non-zero return status means that the command failed.

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

    Single quotes.

  8. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
  2. rbtools/commands/post.py (Diff revision 6)
     
     
    Col: 48
     E502 the backslash is redundant between brackets
    
  3. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
  2. 
      
VI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    Ignored Files:
        .reviewboardrc
    
    
  2. 
      
brennie
  1. 
      
  2. .reviewboardrc (Diff revision 8)
     
     

    You removed the newline from the end of this line. Can you revert this change please?

  3. rbtools/commands/post.py (Diff revision 8)
     
     

    Can we just import subprocess and use subprocess.Popen and subprocess.PIPE ?

  4. rbtools/commands/post.py (Diff revision 8)
     
     
     
     

    This can be simplified to use an and.

    See previous comment re status codes vs output.

  5. 
      
VI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
  2. rbtools/commands/post.py (Diff revision 9)
     
     
    Col: 23
     E128 continuation line under-indented for visual indent
    
  3. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. rbtools/commands/post.py (Diff revision 10)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
    1. Please fix this issue.

  3. 
      
brennie
  1. 
      
  2. rbtools/commands/post.py (Diff revision 10)
     
     

    should we use stdout and stderror here so the user can see messages?

    1. I don't understand your comment. Like we check if the return code is non-zero and print out stdout and stderr if they're non-empty?

    2. if you use subprocess.PIPE, all the output from the exeucted command will be captured and the user won't be able to see it unless we print it.

      However, you can use subprocess.STDOUT to output both standard output and standard error streams to the console.

  3. 
      
gmyers
  1. 
      
  2. rbtools/commands/post.py (Diff revision 10)
     
     
     
     
     
     

    Add added_in field to indicate the version of RBTools where this option was introduced.

    1. how can I find out what the current version is?

    2. This is based off of master so the next version is 0.8.0

  3. rbtools/commands/post.py (Diff revision 10)
     
     
    I don't think this change was intended?
  4. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. 
      
david
  1. 
      
  2. rbtools/commands/post.py (Diff revision 11)
     
     
     

    su before sy

  3. rbtools/commands/post.py (Diff revision 11)
     
     

    This looks like debug output?

  4. rbtools/commands/post.py (Diff revision 11)
     
     

    Can we print out a message saying "X command returned an error" before asking for confirmation?

  5. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. rbtools/commands/post.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (88 > 79 characters)
    
  3. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. rbtools/commands/post.py (Diff revision 13)
     
     
    Col: 31
     W503 line break before binary operator
    
  3. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. rbtools/commands/post.py (Diff revision 14)
     
     
    Col: 61
     W291 trailing whitespace
    
  3. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. 
      
SU
  1. 
      
  2. rbtools/commands/post.py (Diff revision 15)
     
     

    Needs a trailing comma?

    1. Why do we need a trailing comma there?

    2. A convention for Python lists is to include the trailing comma for the last element, similar to line 97. I believe we are following that standard in RB.

  3. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. rbtools/commands/post.py (Diff revision 16)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. rbtools/commands/post.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. 
      
david
Review request changed

Status: Discarded

Loading...