Adding pre-post hook to .reviewboardrc

Review Request #7066 - Created March 16, 2015 and updated

Tran Nguyen
RBTools
master
b29a119...
rbtools

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.

  • 0
  • 0
  • 38
  • 2
  • 40
Description From Last Updated
Review Bot
  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. 
      
Barret Rennie
  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. 
      
Tran Nguyen
Review Bot
  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. 
      
Tran Nguyen
Review Bot
  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. 
      
Tran Nguyen
Review Bot
  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. 
      
Vincent Le
  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. 
      
Tien Vu
  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. 
      
Tran Nguyen
Review Bot
  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. 
      
Barret Rennie
  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. 
      
Tran Nguyen
Review Bot
  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. 
      
Tran Nguyen
Review Bot
  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. 
      
Tran Nguyen
Review Bot
  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. 
      
Barret Rennie
  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. 
      
Tran Nguyen
Review Bot
  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. 
      
Tran Nguyen
Review Bot
  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)
    
  3. 
      
Barret Rennie
  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. 
      
Griffin Myers
  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. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. 
      
David Trowbridge
  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. 
      
Tran Nguyen
Review Bot
  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. 
      
Tran Nguyen
Review Bot
  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. 
      
Tran Nguyen
Review Bot
  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. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. 
      
Wang Jun Sun
  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. 
      
Tran Nguyen
Review Bot
  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. 
      
Tran Nguyen
Review Bot
  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. 
      
Tran Nguyen
Review request changed

Commit:

-d434591a2dac694e483f99de5a6a5542b9042a55
+b29a119900d98517f2c055f190ece9a96f545d56

Diff:

Revision 18 (+21)

Show changes

Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. 
      
Loading...