Summary

Adding pre-post hook to .reviewboardrc

Review Request #7066 — Created March 16, 2015 and discarded Jan. 29, 2019, 1:42 p.m.

Information

Owner

viettran1989

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
PEP8? I'm not sure if we want to set thi in the global .reviewboardrc.	brennie	March 16, 2015, 5:23 p.m.
We use single quotes.	brennie	March 16, 2015, 4:54 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 16, 2015, 4:55 p.m.
Using the shell would save a lot of headaches and make the pre-post hooks more effective and powerful in my …	brennie	March 16, 2015, 4:54 p.m.
Blank line between these.	brennie	March 16, 2015, 4:40 p.m.
Blank line between these.	brennie	March 16, 2015, 4:40 p.m.
Blank line between these.	brennie	March 16, 2015, 4:54 p.m.
This will not work in the case of quoted substrings.	brennie	March 16, 2015, 4:54 p.m.
You can use rbtools.utils.console.confirm instead of rolling your own confirmation logic.	brennie	March 16, 2015, 4:54 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	March 16, 2015, 4:55 p.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	March 16, 2015, 4:55 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	March 16, 2015, 4:54 p.m.
Col: 9 E122 continuation line missing indentation or outdented	reviewbot	March 16, 2015, 5:20 p.m.
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	March 30, 2015, 5:49 p.m.
Use single quotes instead of double quotes. And shouldn't is_continue be continue?	VT VTL-Developer	March 30, 2015, 6:24 p.m.
you can probably just do: if not confirm()	TI tienv	March 30, 2015, 6:24 p.m.
I don't know if we want to force this on our users.	brennie	April 25, 2015, 1:36 p.m.
Missing trailing comma.	brennie	April 25, 2015, 1:27 p.m.
Missing trailing comma.	brennie	April 25, 2015, 1:27 p.m.
This should be formatted as: if ('PRE_POST_HOOK' in self.config and not self.options.no_pre_post_hook): # ...	brennie	April 25, 2015, 1:28 p.m.
Should the result of the influence being able to post ?	brennie	April 28, 2015, 5:25 p.m.
Single quotes.	brennie	April 25, 2015, 1:36 p.m.
Col: 48 E502 the backslash is redundant between brackets	reviewbot	April 25, 2015, 1:49 p.m.
You removed the newline from the end of this line. Can you revert this change please?	brennie	April 28, 2015, 5:15 p.m.
Can we just import subprocess and use subprocess.Popen and subprocess.PIPE ?	brennie	April 28, 2015, 5:15 p.m.
This can be simplified to use an and. See previous comment re status codes vs output.	brennie	April 28, 2015, 5:25 p.m.
Col: 23 E128 continuation line under-indented for visual indent	reviewbot	April 29, 2015, 6:34 a.m.
Add added_in field to indicate the version of RBTools where this option was introduced.	gmyers	May 2, 2015, 2:59 p.m.
I don't think this change was intended?	gmyers	April 30, 2015, 11:33 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	April 29, 2015, 7:01 a.m.
should we use stdout and stderror here so the user can see messages?	brennie	May 2, 2015, 5:53 p.m.
su before sy	david	May 7, 2015, 9:10 a.m.
This looks like debug output?	david	May 7, 2015, 9:10 a.m.
Can we print out a message saying "X command returned an error" before asking for confirmation?	david	May 7, 2015, 9:16 a.m.
Col: 80 E501 line too long (88 > 79 characters)	reviewbot	May 7, 2015, 10:50 a.m.
Col: 31 W503 line break before binary operator	reviewbot	May 10, 2015, 6:28 p.m.
Col: 61 W291 trailing whitespace	reviewbot	May 10, 2015, 6:32 p.m.
Needs a trailing comma?	SU Sunxperous	May 11, 2015, 8:54 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	May 13, 2015, 8:05 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	May 13, 2015, 8:06 a.m.

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

Ignored Files:
    .reviewboardrc



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

Ignored Files:
    .reviewboardrc

rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (84 > 79 characters)
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

<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>

brennie March 16, 2015, 5:24 p.m.

...when the tests fail?

Sorry I apparently got sidetracked and missed finishing the comment.

.reviewboardrc (Diff revision 1)

The issue has been resolved. Show all issues

PEP8?

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

viettran1989 March 16, 2015, 5:05 p.m.

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

brennie March 16, 2015, 5:24 p.m.

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

brennie March 16, 2015, 5:25 p.m.

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

rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We use single quotes.
```

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

The issue has been resolved. Show all issues

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

rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This will not work in the case of quoted substrings.
```

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

The issue has been resolved. Show all issues

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

Diff: