Summary

Add Stage field to a review request

Review Request #8495 — Created Oct. 26, 2016 and discarded Jan. 29, 2019, 1:46 p.m.

Information

Owner

anni_cao

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

This feature will add a stage field to a review request that will provide some
context as to where a review request is in its development/review lifecycle.
This field provides visual information that will aid decision making on a
review request like whether to perform an automatic code review or wait for
the author to make changes.

This feature adds built-in stages which are
- ApprovedStage
- CompletedStage
- DiscardedStage
- PendingChangesStage
- PendingReviewsStage
- UnpublishedStage
- WIPStage

Testing Done

All tests pass

Issues

Description	From	Last Updated
This is looking pretty solid. You will probably want to move on to writing unit tests.	brennie	Nov. 22, 2016, 2:59 p.m.
Missing trailing commma.	brennie	Oct. 28, 2016, 11:59 a.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Oct. 28, 2016, 11:57 a.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Oct. 28, 2016, 11:56 a.m.
Col: 1 W391 blank line at end of file	reviewbot	Oct. 28, 2016, 11:56 a.m.
Single quotes.	brennie	Oct. 28, 2016, 11:59 a.m.
Use the class itself, UnpublishedStage, instead of the instance.	brennie	Oct. 28, 2016, 11:59 a.m.
This should be formatted like a docstring, i.e. #: Single line summary #: #: Multi-line description should_review = False	brennie	Oct. 28, 2016, noon
Move to builtin_stages.py	brennie	Oct. 28, 2016, 12:04 p.m.
MOve this to __init__.py	brennie	Oct. 28, 2016, 12:04 p.m.
This is unnecessary becuase of get_defaults	brennie	Oct. 28, 2016, 12:01 p.m.
Move to builtin_stages.py	brennie	Oct. 28, 2016, 12:04 p.m.
Move to builtin_stages.py	brennie	Oct. 28, 2016, 12:04 p.m.
Move to builtin_stages.py	brennie	Oct. 28, 2016, 12:04 p.m.
Move to builtin_stages.py	brennie	Oct. 28, 2016, 12:04 p.m.
Move to builtin_stages.py	brennie	Oct. 28, 2016, 12:04 p.m.
Move to builtin_stages.py	brennie	Oct. 28, 2016, 12:04 p.m.
Two blank lines between these. Missing a file docstring. Missing from __future__ import unicode literals.	brennie	Oct. 29, 2016, 9:15 p.m.
Not sure why this is here.	brennie	Oct. 29, 2016, 9:16 p.m.
Misisng file-level docstring. Missing from __future__ import unicode literals.	brennie	Oct. 29, 2016, 9:17 p.m.
Docstring.	brennie	Oct. 29, 2016, 9:22 p.m.
Dont need spacing between these. Same below.	brennie	Oct. 29, 2016, 9:22 p.m.
Docstring for all these.	brennie	Oct. 29, 2016, 9:22 p.m.
Misisng file-level docstring. Missing from __future__ import unicode literals.	brennie	Oct. 29, 2016, 9:24 p.m.
Docstring.	brennie	Oct. 29, 2016, 9:25 p.m.
Misisng file-level docstring. Missing from __future__ import unicode literals.	brennie	Oct. 29, 2016, 9:25 p.m.
Col: 38 E128 continuation line under-indented for visual indent	reviewbot	Nov. 14, 2016, 7:15 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Nov. 14, 2016, 7:15 p.m.
Col: 80 E501 line too long (87 > 79 characters)	reviewbot	Nov. 14, 2016, 7:16 p.m.
Args/kwargs missing.	brennie	Nov. 19, 2016, 8:50 a.m.
You also need a hook to add stages from extensions. See https://reviews.reviewboard.org/r/8530/ and https://reviews.reviewboard.org/r/8531/.	brennie	Nov. 19, 2016, 7:49 a.m.
The first import should be from __future__ import unicode_literals.	brennie	Nov. 15, 2016, 10:43 a.m.
Blank line between these.	brennie	Nov. 15, 2016, 10:45 a.m.
"""The global stage registry."""	brennie	Nov. 15, 2016, 10:46 a.m.
stages Also, could you give a quick doc-comment before this (like #: ...).	brennie	Nov. 15, 2016, 10:48 a.m.
This needs to explain the contents of the file.	brennie	Nov. 15, 2016, 11:05 a.m.
Please elaborate on these docstrings, such as when they will be applied to review requests.	brennie	Nov. 15, 2016, 11:05 a.m.
This should be localized: from django.utils.translation import ugettext_lazy as _ # ... class UnpublishedStage(BaseReviewRequestStage): name = _('Unpublished') # ... Same …	brennie	Nov. 15, 2016, 11:07 a.m.
Wrong docstring.	brennie	Nov. 15, 2016, 11:08 a.m.
Please use proper capitilization and punctuation.	brennie	Nov. 15, 2016, 11:09 a.m.
Docstring.	brennie	Nov. 15, 2016, 11:11 a.m.
You're missing reply_published. Also, how about: ```python signals = ( (review_request_published, self._on_review_request_published), (review_request_closed, self._on_review_request_closed), (review_request_reopened, self._on_review_request_reopened), (review_published, self._on_review_published), (reply_published, self._on_reply_published), …	brennie	Nov. 15, 2016, 11:16 a.m.
_on_review_request_published.	brennie	Nov. 15, 2016, 11:17 a.m.
review_request_published will be triggered whenever the review request is published, so this may happen more than once. The first publish …	brennie	Nov. 15, 2016, 11:29 a.m.
Col: 38 E128 continuation line under-indented for visual indent	reviewbot	Nov. 14, 2016, 7:16 p.m.
This needs to explain the contents of the file.	brennie	Nov. 15, 2016, 11:29 a.m.
Can you explain what this is.	brennie	Nov. 15, 2016, 12:05 p.m.
Can you add a get_stage(stage_id) method?	brennie	Nov. 15, 2016, noon
Missing docstring.	brennie	Nov. 15, 2016, 11:40 a.m.
You can just return a list here.	brennie	Nov. 15, 2016, 11:35 a.m.
This needs to explain the contents of the file.	brennie	Nov. 15, 2016, 11:37 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Nov. 19, 2016, 8:02 a.m.
Col: 45 E127 continuation line over-indented for visual indent	reviewbot	Nov. 19, 2016, 8:02 a.m.
Col: 29 E126 continuation line over-indented for hanging indent	reviewbot	Nov. 19, 2016, 8:03 a.m.
Col: 1 W391 blank line at end of file	reviewbot	Nov. 19, 2016, 8:04 a.m.
If review.request.stage returns None, this will raise an AttributeError.	brennie	Nov. 19, 2016, 9:55 a.m.
Col: 80 E501 line too long (95 > 79 characters)	reviewbot	Nov. 19, 2016, 8:33 a.m.
missing **kwargs in Args:	brennie	Nov. 19, 2016, 9:58 a.m.
Missing file level docstring.	brennie	Nov. 19, 2016, 10:01 a.m.
Docstring.	brennie	Nov. 19, 2016, 10:04 a.m.
Missing docstring.	brennie	Nov. 19, 2016, 10:13 a.m.
Should this perhaps compute the stage of the review request if self._stage is None?	brennie	Nov. 19, 2016, 11:14 a.m.
These all need translation.	brennie	Nov. 19, 2016, 10:42 a.m.
missing a space	brennie	Nov. 19, 2016, 10:14 a.m.
"completed"	brennie	Nov. 19, 2016, 10:14 a.m.
Docstrings here and elsewhere.	brennie	Nov. 19, 2016, 10:33 a.m.
No blank line	brennie	Nov. 19, 2016, 10:33 a.m.
Should we perhaps do this before calling self._calculate_review_request_stage? If a hook returns a stage, we throw the value computed by …	brennie	Nov. 19, 2016, 10:34 a.m.
('_stage',) Using a tuple requires less memory allocation.	brennie	Nov. 19, 2016, 10:35 a.m.
Missing period. If this is a generator this should be "Yield the registered stage classes." If this is not it …	brennie	Nov. 19, 2016, 10:37 a.m.
You can just return this instead of yielding.	brennie	Nov. 19, 2016, 12:45 p.m.
Use the full import path.	brennie	Nov. 21, 2016, 12:32 p.m.
Blank line between these.	brennie	Nov. 21, 2016, 12:32 p.m.
django.utils.safestring.SafeText	brennie	Nov. 21, 2016, 12:33 p.m.
No blank line.	brennie	Nov. 21, 2016, 12:33 p.m.
This is indented too far. And also incorrect. These are just extra arguments for the signal handler.	brennie	Nov. 21, 2016, 12:35 p.m.
No blank line.	brennie	Nov. 21, 2016, 12:35 p.m.
Two blank lines.	brennie	Nov. 21, 2016, 12:36 p.m.
Blank line.	brennie	Nov. 21, 2016, 12:38 p.m.
This should be reviewboard.reviews.stages.stages.BaseReviewRequestStage.	brennie	Nov. 21, 2016, 12:39 p.m.
Docstrings for all these.	brennie	Nov. 21, 2016, 5:08 p.m.
This is going to need many unit tests.	brennie	Nov. 22, 2016, 2:59 p.m.
No blank line.	brennie	Nov. 21, 2016, 12:40 p.m.
if not new_stage	brennie	Nov. 21, 2016, 12:40 p.m.
Should be :py:class:`.... Also the :py:class bit needs to be on the same line as the class bit. It is …	brennie	Nov. 21, 2016, 12:45 p.m.
No blank line.	brennie	Nov. 21, 2016, 12:46 p.m.
"for review requests."	brennie	Nov. 21, 2016, 12:47 p.m.
Missing Returns.	brennie	Nov. 21, 2016, 4:55 p.m.
Just return the tuple.	brennie	Nov. 21, 2016, 4:55 p.m.
Can you rename this to base.py? That's usually where we put base classes.	brennie	Nov. 21, 2016, 12:50 p.m.
Col: 27 E128 continuation line under-indented for visual indent	reviewbot	Nov. 22, 2016, 3:29 p.m.
Col: 80 E501 line too long (117 > 79 characters)	reviewbot	Nov. 22, 2016, 3:31 p.m.
Col: 80 E501 line too long (116 > 79 characters)	reviewbot	Nov. 22, 2016, 3:32 p.m.
Col: 80 E501 line too long (121 > 79 characters)	reviewbot	Nov. 22, 2016, 3:32 p.m.
Col: 1 E303 too many blank lines (3)	reviewbot	Nov. 22, 2016, 3:30 p.m.
Missing a period. Docstrings should be complete sentences.	brennie	Nov. 30, 2016, 1:55 p.m.
Should end in a period.	david	Nov. 30, 2016, 10:02 a.m.
Description should go on the next line.	david	Nov. 30, 2016, 10:02 a.m.
Should be imperative mood ("Return" instead of "Returns"), and needs a "Returns" section.	david	Nov. 30, 2016, 10:03 a.m.
Needs a period.	david	Nov. 30, 2016, 10:03 a.m.
You don't need the u prefix on the string literals because we import unicode_literals at the top.	david	Nov. 30, 2016, 10:04 a.m.
Can we alphabetize these?	david	Nov. 30, 2016, 10:05 a.m.
Should be the imperative mood ("Return ..." instead of "Returns ...")	david	Nov. 30, 2016, 10:06 a.m.
In "Returns", the description doesn't need to be indented relative to the type.	david	Nov. 30, 2016, 10:06 a.m.
Should be capitalized and imperative mood ("Set" instead of "sets")	david	Nov. 30, 2016, 10:07 a.m.
In "Returns", the description doesn't need to be indented relative to the type.	david	Nov. 30, 2016, 10:08 a.m.
Can we use the symbols instead of the string literals? In this case, instead of 'S', use ReviewRequest.SUBMITTED.	david	Nov. 30, 2016, 12:29 p.m.
Same here.	david	Nov. 30, 2016, 12:29 p.m.
Can review_request.status be something other than S, D and P?	ST stensby	Nov. 30, 2016, 10 a.m.
Same here.	david	Nov. 30, 2016, 12:29 p.m.
Add a period.	david	Nov. 30, 2016, 10:08 a.m.
Do you need this many comments for the variables? Most of them seem very self-explanatory.	ST stensby	Nov. 30, 2016, 10 a.m.
This description is a little circular. How about "The Review Request has not yet been published"? Also, do we want …	david	Nov. 30, 2016, 10:08 a.m.
"Waiting on a review" is a little confusing. Maybe "The author is waiting for reviews"?	david	Nov. 30, 2016, 10:09 a.m.
Maybe "Waiting for the author to make requested changes"?	david	Nov. 30, 2016, 10:09 a.m.
Description should go on the next line.	david	Nov. 30, 2016, 10:10 a.m.
Description should go on the next line.	david	Nov. 30, 2016, 10:10 a.m.
Description should go on the next line.	david	Nov. 30, 2016, 10:10 a.m.
This sentence doesn't really make sense. Maybe "Whether the review request is being published for the first time"?	david	Nov. 30, 2016, 10:11 a.m.
This sentence doesn't really make sense. Maybe "Whether the review request is being published for the first time"?	david	Nov. 30, 2016, 10:11 a.m.
In "Returns" sections, you don't need to indent the description relative to the type.	david	Nov. 30, 2016, 10:12 a.m.
Instead of using the character to continue the statement, please wrap the whole conditional in parentheses. We can also use …	david	Nov. 30, 2016, 10:14 a.m.
Alphabetize?	david	Nov. 30, 2016, 10:16 a.m.
This should be written in the imperative mood ("Return the registered...") and include a "Returns" section.	david	Nov. 30, 2016, 10:18 a.m.
Same here. Also needs to end in a period.	david	Nov. 30, 2016, 10:18 a.m.
Needs a period.	david	Nov. 30, 2016, 10:19 a.m.
Alphabetize?	david	Nov. 30, 2016, 10:21 a.m.
In test docstrings, we don't add the normal period at the end because the test suite adds "...". Here and …	david	Nov. 30, 2016, 10:22 a.m.
Should be two blank lines here.	david	Dec. 7, 2016, 11:36 a.m.
You don't need a blank line after docstrings for methods (only the docstring for classes).	david	Dec. 7, 2016, 11:37 a.m.
Mind adding another blank line here?	david	Dec. 7, 2016, 11:37 a.m.
It looks like there's some overlap with ReviewRequestStageController.calculate_review_request_stage?	david	Dec. 10, 2016, 12:28 a.m.
We should move this to BaseReviewRequestDetails too.	david	Dec. 7, 2016, 11:40 a.m.
Add a blank line here.	david	Dec. 7, 2016, 11:40 a.m.
"review request" should be sentence case here. Let's also add a period to the end of all of these.	david	Dec. 7, 2016, 11:43 a.m.
Let's make all of these consistent in their language: "The review request ...". Here and below.	david	Dec. 7, 2016, 11:43 a.m.
Dedent these 4 spaces.	david	Dec. 7, 2016, 8:01 p.m.
Here too.	david	Dec. 7, 2016, 8:01 p.m.
And here, and below.	david	Dec. 7, 2016, 8:01 p.m.
review_request.stage should be vertically aligned with first_publish.	david	Dec. 7, 2016, 11:46 a.m.
These can be one statement (just return the tuple)	david	Dec. 7, 2016, 11:48 a.m.
Missing docs on Args.	chipx86	Dec. 9, 2016, 3:27 p.m.
No indentation here for the description.	chipx86	Dec. 9, 2016, 3:27 p.m.
This should check for a value more generally, rather than None, since it can be ''. if review_request.stage: ...	chipx86	Dec. 9, 2016, 3:28 p.m.
Let's also include this in the list of default columns.	chipx86	Dec. 9, 2016, 3:35 p.m.
Should be reviewboard.extensions.base.Extension.	chipx86	Dec. 9, 2016, 3:35 p.m.
There can't be a blank line here. Also, it must be :py:class:, not py:class:	chipx86	Dec. 9, 2016, 3:37 p.m.
Docstrings are only valuable when they convey something the user doesn't already know. This just says what __init__.py is, generally. …	chipx86	Dec. 9, 2016, 3:42 p.m.
I would expect this to live in reviewboard/reviews/stages/controller.py, with a setter and getter (like we have for other singleton instances).	chipx86	Dec. 10, 2016, 9:21 p.m.
Needs a trailing comma.	chipx86	Dec. 9, 2016, 3:46 p.m.
This should remain only in ReviewRequest.	chipx86	Dec. 9, 2016, 3:50 p.m.
This should remain only in ReviewRequest.	chipx86	Dec. 9, 2016, 3:50 p.m.
This should be a CharField, not a TextField. The latter is meant for larger pieces of text, and won't index …	chipx86	Dec. 10, 2016, 12:27 a.m.
Args is indented too far.	chipx86	Dec. 9, 2016, 3:52 p.m.
`:py:class:	chipx86	Dec. 9, 2016, 3:52 p.m.
This function should go away. All computation is going to happen in the controller. ReviewRequest.__init__ will check if the stage …	chipx86	Dec. 10, 2016, 1:02 a.m.
:py:class:	chipx86	Dec. 9, 2016, 3:55 p.m.
This file is more than just the registry. It's the whole module for working with stages. This specific file just …	chipx86	Dec. 9, 2016, 9:55 p.m.
Read this as if you weren't aware of the stages work, and then see if you can think of some …	chipx86	Dec. 9, 2016, 9:50 p.m.
"built-in"	chipx86	Dec. 9, 2016, 3:57 p.m.
No point in setting the values if they aren't changing from the base class. Can you go through each of …	chipx86	Dec. 9, 2016, 9:36 p.m.
This should include some information on how those updates are applied, and how consumers might override computation.	chipx86	Dec. 9, 2016, 9:42 p.m.
We usually say something like "Handler for review request published signals."	chipx86	Dec. 9, 2016, 9:31 p.m.
This is the wrong module path. Same below.	chipx86	Dec. 9, 2016, 9:45 p.m.
You can combine these into one call: self._update_review_request_stage( review_request, first_publish=changedesc is None)	chipx86	Dec. 9, 2016, 4 p.m.
Should be (bool, optional)	chipx86	Dec. 9, 2016, 4:01 p.m.
Let's use keyword arguments in the call. Helps for future expansion.	chipx86	Dec. 10, 2016, 10:03 p.m.
Blank line between these.	chipx86	Dec. 9, 2016, 4:02 p.m.
:py:class:	chipx86	Dec. 9, 2016, 4:02 p.m.
We don't use title case. This should better summarize what this class is. "A registry for tracking available stages for …	chipx86	Dec. 9, 2016, 4:03 p.m.
Missing period. This should also flesh out the responsibilities of this class a bit more. Remember that, for every docstring, …	chipx86	Dec. 9, 2016, 4:05 p.m.
The docstring isn't correct. This isn't returning registered stages. It's returning the default stages that the registry knows about.	chipx86	Dec. 9, 2016, 9:28 p.m.
A couple things here: The description is redundant. That's implied by the type. Don't just talk return values and data …	chipx86	Dec. 9, 2016, 4:09 p.m.
:py:class:	chipx86	Dec. 9, 2016, 4:10 p.m.
"... registered to the provided stage ID."	chipx86	Dec. 9, 2016, 4:10 p.m.
As above, this should be talking about what's truly being returned beyond just a data type.	chipx86	Dec. 9, 2016, 9:28 p.m.
A tests.py file should contain unit tests. Given the simplicity of this dummy object, I don't think we should bother …	chipx86	Dec. 10, 2016, 10:02 p.m.
Test docstrings should mention the classes/attribute/method/etc. being worked with. However, these tests are all testing different classes, which is often …	chipx86	Dec. 10, 2016, 10:02 p.m.
I wouldn't expect this call to be necessary? That should be calculated on access.	chipx86	Dec. 10, 2016, 10:02 p.m.
Col: 1 W391 blank line at end of file	reviewbot	Dec. 10, 2016, 10:34 p.m.
'DiscardedStage' imported but unused	reviewbot	Dec. 10, 2016, 10:34 p.m.
'CompletedStage' imported but unused	reviewbot	Dec. 10, 2016, 10:34 p.m.
'PendingReviewsStage' imported but unused	reviewbot	Dec. 10, 2016, 10:34 p.m.
'UnpublishedStage' imported but unused	reviewbot	Dec. 10, 2016, 10:34 p.m.
'WIPStage' imported but unused	reviewbot	Dec. 10, 2016, 10:35 p.m.
Col: 1 W391 blank line at end of file	reviewbot	Dec. 10, 2016, 10:35 p.m.
Col: 1 W391 blank line at end of file	reviewbot	Dec. 10, 2016, 10:36 p.m.
'DiscardedStage' imported but unused	reviewbot	Dec. 10, 2016, 10:36 p.m.
'CompletedStage' imported but unused	reviewbot	Dec. 10, 2016, 10:36 p.m.
'PendingReviewsStage' imported but unused	reviewbot	Dec. 10, 2016, 10:36 p.m.
'UnpublishedStage' imported but unused	reviewbot	Dec. 10, 2016, 10:36 p.m.
'WIPStage' imported but unused	reviewbot	Dec. 10, 2016, 10:36 p.m.
Col: 1 W391 blank line at end of file	reviewbot	Dec. 10, 2016, 10:36 p.m.
This should actually be None initially.	brennie	Jan. 21, 2017, 10:13 a.m.
You need to do: from reviewboard.reviews.stages import stage_controller # ... stage_controller.calculate_review_request_stage(self.get_review_request())	brennie	Jan. 22, 2017, 9:49 a.m.
This shouldn't be here.	brennie	Jan. 22, 2017, 9:48 a.m.
redefinition of unused 'TestCase' from line 55	reviewbot	Jan. 22, 2017, 10:08 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/reviews/stages/registry.py
    reviewboard/reviews/stages/stage.py
    reviewboard/reviews/models/base_review_request_details.py

Ignored Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/stages/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/reviews/stages/registry.py
    reviewboard/reviews/stages/stage.py
    reviewboard/reviews/models/base_review_request_details.py

Ignored Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/stages/__init__.py

reviewboard/reviews/evolutions/add_stages.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/reviews/evolutions/add_stages.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
reviewboard/reviews/evolutions/add_stages.py (Diff revision 1)
Show all issues
```
Col: 1
 W391 blank line at end of file
```

reviewboard/reviews/evolutions/__init__.py (Diff revision 1)
Show all issues
```
Missing trailing commma.
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 1)
Show all issues
```
Single quotes.
```

reviewboard/reviews/stages/registry.py (Diff revision 1)

Show all issues

Use the class itself, UnpublishedStage, instead of the instance.

reviewboard/reviews/stages/stage.py (Diff revision 1)

Show all issues

This should be formatted like a docstring, i.e.

#: Single line summary
#:
#: Multi-line description
should_review = False

reviewboard/reviews/stages/stage.py (Diff revision 1)
Show all issues
```
Move to builtin_stages.py
```
reviewboard/reviews/stages/stage.py (Diff revision 1)
Show all issues
```
MOve this to __init__.py
```
reviewboard/reviews/stages/stage.py (Diff revision 1)
Show all issues
```
This is unnecessary becuase of get_defaults
```
reviewboard/reviews/stages/stage.py (Diff revision 1)
Show all issues
```
Move to builtin_stages.py
```
reviewboard/reviews/stages/stage.py (Diff revision 1)
Show all issues
```
Move to builtin_stages.py
```
reviewboard/reviews/stages/stage.py (Diff revision 1)
Show all issues
```
Move to builtin_stages.py
```
reviewboard/reviews/stages/stage.py (Diff revision 1)
Show all issues
```
Move to builtin_stages.py
```
reviewboard/reviews/stages/stage.py (Diff revision 1)
Show all issues
```
Move to builtin_stages.py
```
reviewboard/reviews/stages/stage.py (Diff revision 1)
Show all issues
```
Move to builtin_stages.py
```

Change Summary:

fixing stage folder issues

Diff:

Revision 2 (+167)

Show changes

	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/reviews/stages/__init__.py
	reviewboard/reviews/stages/builtin_stages.py
	reviewboard/reviews/stages/registry.py
	reviewboard/reviews/stages/stage.py

Summary:

-	Add Stage field to a review request
+	WIP: Add Stage field to a review request

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/reviews/stages/registry.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/stage.py
    reviewboard/reviews/models/base_review_request_details.py



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/reviews/stages/registry.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/stage.py
    reviewboard/reviews/models/base_review_request_details.py

reviewboard/reviews/stages/__init__.py (Diff revision 2)

Show all issues

Two blank lines between these.

Missing a file docstring.

Missing from __future__ import unicode literals.

reviewboard/reviews/stages/__init__.py (Diff revision 2)
Show all issues
```
Not sure why this is here.
```

reviewboard/reviews/stages/builtin_stages.py (Diff revision 2)

Show all issues

Misisng file-level docstring.

Missing from __future__ import unicode literals.

reviewboard/reviews/stages/builtin_stages.py (Diff revision 2)
Show all issues
```
Docstring.
```
reviewboard/reviews/stages/builtin_stages.py (Diff revision 2)
Show all issues
```
Dont need spacing between these. Same below.
```
reviewboard/reviews/stages/builtin_stages.py (Diff revision 2)
Show all issues
```
Docstring for all these.
```

reviewboard/reviews/stages/registry.py (Diff revision 2)

Show all issues

Misisng file-level docstring.

Missing from __future__ import unicode literals.

reviewboard/reviews/stages/registry.py (Diff revision 2)
Show all issues
```
Docstring.
```

reviewboard/reviews/stages/stage.py (Diff revision 2)

Show all issues

Misisng file-level docstring.

Missing from __future__ import unicode literals.

Change Summary:

new controller chanegs and integration

Commit:

-	cf11b3bbdb271ab902899766d1729a5ac4395687
+	627e2b3b62ab07ae567bfd2cbcc92ffc31bbcd57

Diff:

Revision 3 (+250)

Show changes

	reviewboard/reviews/__init__.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/reviews/stages/__init__.py
	reviewboard/reviews/stages/builtin_stages.py
	reviewboard/reviews/stages/controller.py
	reviewboard/reviews/stages/registry.py
	2 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/reviews/stages/registry.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/stages/stage.py
    reviewboard/reviews/models/base_review_request_details.py

Ignored Files:
    reviewboard/templates/reviews/review_request_box.html



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/reviews/stages/registry.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/stages/stage.py
    reviewboard/reviews/models/base_review_request_details.py

Ignored Files:
    reviewboard/templates/reviews/review_request_box.html

reviewboard/reviews/stages/controller.py (Diff revision 3)

Show all issues

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

reviewboard/reviews/stages/controller.py (Diff revision 3)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/stages/controller.py (Diff revision 3)
Show all issues
```
Col: 80
 E501 line too long (87 > 79 characters)
```

Change Summary:

added UI option for stages and prilminary hook functionality

Commit:

-	627e2b3b62ab07ae567bfd2cbcc92ffc31bbcd57
+	b9b60eff6072801c4b24d6ef50733a4914554da5

Diff:

Revision 4 (+313)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/datagrids/grids.py
	reviewboard/extensions/hooks.py
	reviewboard/reviews/__init__.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	reviewboard/reviews/models/base_review_request_details.py
	5 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/stage.py
    reviewboard/reviews/models/base_review_request_details.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/stage.py
    reviewboard/reviews/models/base_review_request_details.py

reviewboard/reviews/stages/controller.py (Diff revision 4)

Show all issues

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

reviewboard/datagrids/columns.py (Diff revision 4)
Show all issues
```
Args/kwargs missing.
```
1. LA
  
  larmiej Nov. 19, 2016, 7:51 a.m.
  I don't understand what you mean, should I add in the args into the comment?
2. brennie Nov. 19, 2016, 8:42 a.m.
  Yes.

reviewboard/extensions/hooks.py (Diff revision 4)

Show all issues

You also need a hook to add stages from extensions. See https://reviews.reviewboard.org/r/8530/ and https://reviews.reviewboard.org/r/8531/.

reviewboard/reviews/__init__.py (Diff revision 4)

Show all issues

The first import should be from __future__ import unicode_literals.

reviewboard/reviews/builtin_fields.py (Diff revision 4)
Show all issues
```
Blank line between these.
```
reviewboard/reviews/stages/__init__.py (Diff revision 4)
Show all issues
```
"""The global stage registry."""
```

reviewboard/reviews/stages/__init__.py (Diff revision 4)

Show all issues

stages
Also, could you give a quick doc-comment before this (like #: ...).

reviewboard/reviews/stages/builtin_stages.py (Diff revision 4)
Show all issues
```
This needs to explain the contents of the file.
```

reviewboard/reviews/stages/builtin_stages.py (Diff revision 4)

Show all issues

Please elaborate on these docstrings, such as when they will be applied to review requests.

reviewboard/reviews/stages/builtin_stages.py (Diff revision 4)

Show all issues

This should be localized:


from django.utils.translation import ugettext_lazy as _


# ...

class UnpublishedStage(BaseReviewRequestStage):
    name = _('Unpublished')
    # ...


Same for all stages.

reviewboard/reviews/stages/builtin_stages.py (Diff revision 4)
Show all issues
```
Wrong docstring.
```
reviewboard/reviews/stages/controller.py (Diff revision 4)
Show all issues
```
Please use proper capitilization and punctuation.
```
reviewboard/reviews/stages/controller.py (Diff revision 4)
Show all issues
```
Docstring.
```

reviewboard/reviews/stages/controller.py (Diff revision 4)

Show all issues

You're missing reply_published. Also, how about:
```python

signals = (

    (review_request_published, self._on_review_request_published),

    (review_request_closed, self._on_review_request_closed),

    (review_request_reopened, self._on_review_request_reopened),

    (review_published, self._on_review_published),

    (reply_published, self._on_reply_published),

)
for signal, handler in signals:

    signal.connect(handler)

reviewboard/reviews/stages/controller.py (Diff revision 4)
Show all issues
```
_on_review_request_published.
```

reviewboard/reviews/stages/controller.py (Diff revision 4)

Show all issues

review_request_published will be triggered whenever the review request is published, so this may happen more than once. The first publish occurs when changedesc is None, however.

reviewboard/reviews/stages/registry.py (Diff revision 4)
Show all issues
```
This needs to explain the contents of the file.
```
reviewboard/reviews/stages/registry.py (Diff revision 4)
Show all issues
```
Can you explain what this is.
```
reviewboard/reviews/stages/registry.py (Diff revision 4)
Show all issues
```
Can you add a get_stage(stage_id) method?
```
reviewboard/reviews/stages/registry.py (Diff revision 4)
Show all issues
```
Missing docstring.
```
reviewboard/reviews/stages/registry.py (Diff revision 4)
Show all issues
```
You can just return a list here.
```
reviewboard/reviews/stages/stage.py (Diff revision 4)
Show all issues
```
This needs to explain the contents of the file.
```

Change Summary:

adding stage hooks

Commit:

-	b9b60eff6072801c4b24d6ef50733a4914554da5
+	7fec0972c9a4c1ce3cfdf087f752f2bd2951fb9a

Diff:

Revision 5 (+403)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/datagrids/grids.py
	reviewboard/extensions/hooks.py
	reviewboard/reviews/__init__.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	reviewboard/reviews/models/base_review_request_details.py
	5 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/stage.py
    reviewboard/reviews/models/base_review_request_details.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/stage.py
    reviewboard/reviews/models/base_review_request_details.py

reviewboard/extensions/hooks.py (Diff revision 5)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

reviewboard/reviews/stages/controller.py (Diff revision 5)

Show all issues

Col: 45
 E127 continuation line over-indented for visual indent

reviewboard/reviews/stages/controller.py (Diff revision 5)

Show all issues

Col: 29
 E126 continuation line over-indented for hanging indent

reviewboard/reviews/stages/registry.py (Diff revision 5)
Show all issues
```
Col: 1
 W391 blank line at end of file
```

Change Summary:

issue fixes and adding proper hook parent class

Commit:

-	7fec0972c9a4c1ce3cfdf087f752f2bd2951fb9a
+	d6f3d3c4951c1b7d81a1b538f4e0174be6d2d1d5

Diff:

Revision 6 (+403 -1)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/datagrids/grids.py
	reviewboard/extensions/hooks.py
	reviewboard/reviews/__init__.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	reviewboard/reviews/models/base_review_request_details.py
	5 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/stage.py
    reviewboard/reviews/models/base_review_request_details.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/stage.py
    reviewboard/reviews/models/base_review_request_details.py

reviewboard/extensions/hooks.py (Diff revision 6)
Show all issues
```
Col: 80
 E501 line too long (95 > 79 characters)
```

reviewboard/datagrids/columns.py (Diff revision 6)

Show all issues

If review.request.stage returns None, this will raise an AttributeError.

reviewboard/extensions/hooks.py (Diff revision 6)
Show all issues
```
missing **kwargs in Args:
```
reviewboard/reviews/__init__.py (Diff revision 6)
Show all issues
```
Missing file level docstring.
```
reviewboard/reviews/__init__.py (Diff revision 6)
Show all issues
```
Docstring.
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 6)
Show all issues
```
Missing docstring.
```

reviewboard/reviews/models/base_review_request_details.py (Diff revision 6)

Show all issues

Should this perhaps compute the stage of the review request if self._stage is None?

reviewboard/reviews/stages/builtin_stages.py (Diff revision 6)
Show all issues
```
These all need translation.
```
reviewboard/reviews/stages/builtin_stages.py (Diff revision 6)
Show all issues
```
missing a space
```
reviewboard/reviews/stages/builtin_stages.py (Diff revision 6)
Show all issues
```
"completed"
```
reviewboard/reviews/stages/controller.py (Diff revision 6)
Show all issues
```
Docstrings here and elsewhere.
```
reviewboard/reviews/stages/controller.py (Diff revision 6)
Show all issues
```
No blank line
```

reviewboard/reviews/stages/controller.py (Diff revision 6)

Show all issues

Should we perhaps do this before calling self._calculate_review_request_stage? If a hook returns a stage, we throw the value computed by _calculate_review_request_stage out anyway.

reviewboard/reviews/stages/controller.py (Diff revision 6)
Show all issues
```
('_stage',)
Using a tuple requires less memory allocation.
```

reviewboard/reviews/stages/registry.py (Diff revision 6)

Show all issues

Missing period.

If this is a generator this should be "Yield the registered stage classes."

If this is not it should be "Return the registered stage classes."

Method docstrings should be in the imperitive mood, i.e., they instruct you as to what they are doing. They should never begin with "this".

reviewboard/reviews/stages/registry.py (Diff revision 6)

Show all issues

You can just return this instead of yielding.

Change Summary:

fixing issues and working on the stage implementation

Commit:

-	d6f3d3c4951c1b7d81a1b538f4e0174be6d2d1d5
+	f0db1821be80e2acc7c7560639eb799a8db96490

Diff:

Revision 7 (+498 -3)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/datagrids/grids.py
	reviewboard/extensions/hooks.py
	reviewboard/reviews/__init__.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	reviewboard/reviews/models/base_review_request_details.py
	5 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/stage.py
    reviewboard/reviews/models/base_review_request_details.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/stage.py
    reviewboard/reviews/models/base_review_request_details.py

Show all issues

This is looking pretty solid. You will probably want to move on to writing unit tests.

reviewboard/datagrids/columns.py (Diff revision 7)
Show all issues
```
Use the full import path.
```
reviewboard/datagrids/columns.py (Diff revision 7)
Show all issues
```
Blank line between these.
```
reviewboard/datagrids/columns.py (Diff revision 7)
Show all issues
```
django.utils.safestring.SafeText
```
reviewboard/datagrids/columns.py (Diff revision 7)
Show all issues
```
No blank line.
```

reviewboard/reviews/__init__.py (Diff revision 7)

Show all issues

This is indented too far. And also incorrect. These are just extra arguments for the signal handler.

reviewboard/reviews/__init__.py (Diff revision 7)
Show all issues
```
No blank line.
```
reviewboard/reviews/__init__.py (Diff revision 7)
Show all issues
```
Two blank lines.
```
reviewboard/reviews/builtin_fields.py (Diff revision 7)
Show all issues
```
Blank line.
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 7)
Show all issues
```
This should be reviewboard.reviews.stages.stages.BaseReviewRequestStage.
```
reviewboard/reviews/stages/controller.py (Diff revision 7)
Show all issues
```
Docstrings for all these.
```
reviewboard/reviews/stages/controller.py (Diff revision 7)
Show all issues
```
This is going to need many unit tests.
```
reviewboard/reviews/stages/controller.py (Diff revision 7)
Show all issues
```
No blank line.
```
reviewboard/reviews/stages/controller.py (Diff revision 7)
Show all issues
```
if not new_stage
```

reviewboard/reviews/stages/controller.py (Diff revision 7)

Show all issues

Should be :py:class:`.... Also the :py:class bit needs to be on the same line as the class bit. It is okay if it goes over 80 chars for cases like this where it is unavaoidable.

reviewboard/reviews/stages/controller.py (Diff revision 7)
Show all issues
```
No blank line.
```
reviewboard/reviews/stages/registry.py (Diff revision 7)
Show all issues
```
"for review requests."
```
reviewboard/reviews/stages/registry.py (Diff revision 7)
Show all issues
```
Missing Returns.
```

reviewboard/reviews/stages/registry.py (Diff revision 7)

Show all issues

Just return the tuple.

reviewboard/reviews/stages/stage.py (Diff revision 7)

Show all issues

Can you rename this to base.py? That's usually where we put base classes.

Change Summary:

Adding unit test for controller and hooks, fixing review issues

Testing Done:

~		None
	~	All tests pass

Commit:

-	f0db1821be80e2acc7c7560639eb799a8db96490
+	714b6d9c3a292bd9007e849671dc12a45bcb7fa5

Diff:

Revision 8 (+692 -3)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/datagrids/grids.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/reviews/__init__.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	8 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/stages/tests.py
    reviewboard/reviews/tests/test_stages.py
    reviewboard/extensions/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/stages/tests.py
    reviewboard/reviews/tests/test_stages.py
    reviewboard/extensions/tests.py

reviewboard/extensions/tests.py (Diff revision 8)

Show all issues

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

reviewboard/reviews/models/base_review_request_details.py (Diff revision 8)
Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 8)
Show all issues
```
Col: 80
 E501 line too long (116 > 79 characters)
```
reviewboard/reviews/stages/controller.py (Diff revision 8)
Show all issues
```
Col: 80
 E501 line too long (121 > 79 characters)
```
reviewboard/reviews/tests/test_stages.py (Diff revision 8)
Show all issues
```
Col: 1
 E303 too many blank lines (3)
```

Change Summary:

fixing bot issues

Commit:

-	714b6d9c3a292bd9007e849671dc12a45bcb7fa5
+	15fb39c6f7109375bcf7e97ce135cd4a5af81ba0

Diff:

Revision 9 (+692 -3)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/datagrids/grids.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/reviews/__init__.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	8 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/stages/tests.py
    reviewboard/reviews/tests/test_stages.py
    reviewboard/extensions/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/stages/tests.py
    reviewboard/reviews/tests/test_stages.py
    reviewboard/extensions/tests.py

Change Summary:

removed WIP

Summary:

-	WIP: Add Stage field to a review request
+	Add Stage field to a review request

reviewboard/reviews/models/base_review_request_details.py (Diff revision 9)

Show all issues

Can review_request.status be something other than S, D and P?

larmiej Nov. 30, 2016, 10:23 a.m.

currently, those are the only options for the status of a review request

reviewboard/reviews/stages/base.py (Diff revision 9)

Show all issues

Do you need this many comments for the variables?  Most of them seem very self-explanatory.

larmiej Nov. 30, 2016, 10:23 a.m.

yeah, these comments were originally detailed as such in the orignal class design. And more information does not hurt

reviewboard/datagrids/columns.py (Diff revision 9)
Show all issues
```
Should end in a period.
```
reviewboard/datagrids/columns.py (Diff revision 9)
Show all issues
```
Description should go on the next line.
```

reviewboard/extensions/hooks.py (Diff revision 9)

Show all issues

Should be imperative mood ("Return" instead of "Returns"), and needs a "Returns" section.

reviewboard/reviews/__init__.py (Diff revision 9)
Show all issues
```
Needs a period.
```

reviewboard/reviews/evolutions/add_stages.py (Diff revision 9)

Show all issues

You don't need the u prefix on the string literals because we import unicode_literals at the top.

reviewboard/reviews/models/base_review_request_details.py (Diff revision 9)
Show all issues
```
Can we alphabetize these?
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 9)
Show all issues
```
Should be the imperative mood ("Return ..." instead of "Returns ...")
```

reviewboard/reviews/models/base_review_request_details.py (Diff revision 9)

Show all issues

In "Returns", the description doesn't need to be indented relative to the type.

reviewboard/reviews/models/base_review_request_details.py (Diff revision 9)
Show all issues
```
Should be capitalized and imperative mood ("Set" instead of "sets")
```

reviewboard/reviews/models/base_review_request_details.py (Diff revision 9)

Show all issues

In "Returns", the description doesn't need to be indented relative to the type.

reviewboard/reviews/models/base_review_request_details.py (Diff revision 9)

Show all issues

Can we use the symbols instead of the string literals? In this case, instead of 'S', use ReviewRequest.SUBMITTED.

larmiej Nov. 30, 2016, 10:27 a.m.

ReviewRequest is a child of base_review_request_details so I believe it may not be able to be imported. I guess i can move the declaration in the child class to the parent class

reviewboard/reviews/models/base_review_request_details.py (Diff revision 9)
Show all issues
```
Same here.
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 9)
Show all issues
```
Same here.
```
reviewboard/reviews/stages/__init__.py (Diff revision 9)
Show all issues
```
Add a period.
```

reviewboard/reviews/stages/builtin_stages.py (Diff revision 9)

Show all issues

This description is a little circular. How about "The Review Request has not yet been published"?

Also, do we want to include periods in these strings?

reviewboard/reviews/stages/builtin_stages.py (Diff revision 9)

Show all issues

"Waiting on a review" is a little confusing. Maybe "The author is waiting for reviews"?

reviewboard/reviews/stages/builtin_stages.py (Diff revision 9)
Show all issues
```
Maybe "Waiting for the author to make requested changes"?
```
reviewboard/reviews/stages/controller.py (Diff revision 9)
Show all issues
```
Description should go on the next line.
```
reviewboard/reviews/stages/controller.py (Diff revision 9)
Show all issues
```
Description should go on the next line.
```
reviewboard/reviews/stages/controller.py (Diff revision 9)
Show all issues
```
Description should go on the next line.
```

reviewboard/reviews/stages/controller.py (Diff revision 9)

Show all issues

This sentence doesn't really make sense. Maybe "Whether the review request is being published for the first time"?

reviewboard/reviews/stages/controller.py (Diff revision 9)

Show all issues

This sentence doesn't really make sense. Maybe "Whether the review request is being published for the first time"?

reviewboard/reviews/stages/controller.py (Diff revision 9)

Show all issues

In "Returns" sections, you don't need to indent the description relative to the type.

reviewboard/reviews/stages/controller.py (Diff revision 9)

Show all issues

Instead of using the character to continue the statement, please wrap the whole conditional in parentheses. We can also use the in operator to combine the second two clauses:

if (first_publish and
    review_request.stage in (None, UnpublishedStage)):

reviewboard/reviews/stages/registry.py (Diff revision 9)
Show all issues
```
Alphabetize?
```

reviewboard/reviews/stages/registry.py (Diff revision 9)

Show all issues

This should be written in the imperative mood ("Return the registered...") and include a "Returns" section.

reviewboard/reviews/stages/registry.py (Diff revision 9)
Show all issues
```
Same here. Also needs to end in a period.
```
reviewboard/reviews/stages/tests.py (Diff revision 9)
Show all issues
```
Needs a period.
```
reviewboard/reviews/tests/test_stages.py (Diff revision 9)
Show all issues
```
Alphabetize?
```

reviewboard/reviews/tests/test_stages.py (Diff revision 9)

Show all issues

In test docstrings, we don't add the normal period at the end because the test suite adds "...". Here and all the other methods in here.

reviewboard/datagrids/columns.py (Diff revision 9)

Show all issues

Missing a period. Docstrings should be complete sentences.

brennie Nov. 30, 2016, 1:56 p.m.

Meant to discard this review but published accidentally.

Change Summary:

fixing review changes mostly doc string corrections

Commit:

-	15fb39c6f7109375bcf7e97ce135cd4a5af81ba0
+	a85a57683f91c2ff8ca30fba507b6b0847690f0c

Diff:

Revision 10 (+718 -10)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/datagrids/grids.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/reviews/__init__.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	9 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/stages/tests.py
    reviewboard/reviews/tests/test_stages.py
    reviewboard/extensions/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/stages/tests.py
    reviewboard/reviews/tests/test_stages.py
    reviewboard/extensions/tests.py

Description:

		This feature will add a stage field to a review request that will
		provide some context as to where a review request is in its
		development/review lifecycle.
-
-		Currently just added a new evolution and working with registries

Change Summary:

updating description

Description:

~		This feature will add a stage field to a review request that will
~		provide some context as to where a review request is in its
~		development/review lifecycle.
	~	This feature will add a stage field to a review request that will provide some
	~	context as to where a review request is in its development/review lifecycle.
	~	This field provides visual information that will aid decision making on a
	+	review request like whether to perform an automatic code review or wait for
	+	the author to make changes.
	+
	+	This feature adds built-in stages which are
	+	- ApprovedStage
	+	- CompletedStage
	+	- DiscardedStage
	+	- PendingChangesStage
	+	- PendingReviewsStage
	+	- UnpublishedStage
	+	- WIPStage

Change Summary:

fixing imports to be alphabetical

Commit:

-	a85a57683f91c2ff8ca30fba507b6b0847690f0c
+	f88371f498174a701d55f0e8ae8cce9ef954430b

Diff:

Revision 11 (+718 -10)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/datagrids/grids.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/reviews/__init__.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	9 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/stages/tests.py
    reviewboard/reviews/tests/test_stages.py
    reviewboard/extensions/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/stages/tests.py
    reviewboard/reviews/tests/test_stages.py
    reviewboard/extensions/tests.py

reviewboard/extensions/hooks.py (Diff revision 11)
Show all issues
```
Should be two blank lines here.
```

reviewboard/extensions/tests.py (Diff revision 11)

Show all issues

You don't need a blank line after docstrings for methods (only the docstring for classes).

reviewboard/reviews/__init__.py (Diff revision 11)
Show all issues
```
Mind adding another blank line here?
```

reviewboard/reviews/models/base_review_request_details.py (Diff revision 11)

Show all issues

It looks like there's some overlap with ReviewRequestStageController.calculate_review_request_stage?

larmiej Dec. 7, 2016, 1:14 p.m.

yeah, this calculates the stage when the stage field is none. It is a pre-process for determing the stage when it has not been set.
The ReviewRequestStageController.calculate_review_request_stage will not accurately calculate the stage when it is none.

david Dec. 9, 2016, 11:48 a.m.

Can we make it so the controller can handle a None stage? It would be nice to not have the duplication.

larmiej Dec. 9, 2016, 11:59 a.m.

if it is None and we go to the controller to calculate it, then we will end up with a recursive function because on retrieval of the review request stage it will try to re-calculate on seeing None. The controller actually does have provision also for if a review request stage is None but the other function was added so if a review request stage was requested, it will have a vaild stage object instead of none.

reviewboard/reviews/models/review_request.py (Diff revision 11)

Show all issues

We should move this to BaseReviewRequestDetails too.

reviewboard/reviews/stages/__init__.py (Diff revision 11)
Show all issues
```
Add a blank line here.
```

reviewboard/reviews/stages/builtin_stages.py (Diff revision 11)

Show all issues

"review request" should be sentence case here. Let's also add a period to the end of all of these.

reviewboard/reviews/stages/builtin_stages.py (Diff revision 11)

Show all issues

Let's make all of these consistent in their language: "The review request ...". Here and below.

reviewboard/reviews/stages/controller.py (Diff revision 11)

Show all issues

Dedent these 4 spaces.

larmiej Dec. 7, 2016, 1:14 p.m.

sorry, i dont understand what you mean. could i have an example?

david Dec. 7, 2016, 1:39 p.m.

Here's what you have:

"""Signal function called when a review request is closed.

    Args:
        review_request (reviewboard.scmtools.models.ReviewRequest):
            The provided review request.

        **kwargs (dict):
            The additional keyword arguments.
"""


Here's what I want:
"""Signal function called when a review request is closed.

Args:
    review_request (reviewboard.scmtools.models.ReviewRequest):
        The provided review request.

    **kwargs (dict):
        The additional keyword arguments.
"""

reviewboard/reviews/stages/controller.py (Diff revision 11)
Show all issues
```
Here too.
```
reviewboard/reviews/stages/controller.py (Diff revision 11)
Show all issues
```
And here, and below.
```

reviewboard/reviews/stages/controller.py (Diff revision 11)

Show all issues

review_request.stage should be vertically aligned with first_publish.

larmiej Dec. 7, 2016, 1:14 p.m.

This may be flagged by the review bot, made the change tho

david Dec. 7, 2016, 1:39 p.m.

Review Bot is configured to ignore this, but if you were running the pep8 or flake8 tools yourself, it would complain.

reviewboard/reviews/stages/registry.py (Diff revision 11)

Show all issues

These can be one statement (just return the tuple)

Change Summary:

fixing review issues on doc strings and a few other minor things

Commit:

-	f88371f498174a701d55f0e8ae8cce9ef954430b
+	b2309fc9d2a160ae16252ba0514eec15332f7728

Diff:

Revision 12 (+723 -15)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/datagrids/grids.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/reviews/__init__.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	9 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/stages/tests.py
    reviewboard/reviews/tests/test_stages.py
    reviewboard/extensions/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/stages/tests.py
    reviewboard/reviews/tests/test_stages.py
    reviewboard/extensions/tests.py

reviewboard/datagrids/columns.py (Diff revision 12)
Show all issues
```
Missing docs on Args.
```
reviewboard/datagrids/columns.py (Diff revision 12)
Show all issues
```
No indentation here for the description.
```

reviewboard/datagrids/columns.py (Diff revision 12)

Show all issues

This should check for a value more generally, rather than None, since it can be ''.

if review_request.stage:
    ...

reviewboard/datagrids/grids.py (Diff revision 12)

Show all issues

Let's also include this in the list of default columns.

reviewboard/extensions/hooks.py (Diff revision 12)
Show all issues
```
Should be reviewboard.extensions.base.Extension.
```

reviewboard/extensions/hooks.py (Diff revision 12)

Show all issues

There can't be a blank line here.

Also, it must be :py:class:, not py:class:

reviewboard/reviews/__init__.py (Diff revision 12)

Show all issues

Docstrings are only valuable when they convey something the user doesn't already know. This just says what __init__.py is, generally. Instead, this should be explaining the purpose of this file.

reviewboard/reviews/__init__.py (Diff revision 12)

Show all issues

I would expect this to live in reviewboard/reviews/stages/controller.py, with a setter and getter (like we have for other singleton instances).

reviewboard/reviews/evolutions/add_stages.py (Diff revision 12)
Show all issues
```
Needs a trailing comma.
```

reviewboard/reviews/models/base_review_request_details.py (Diff revision 12)

Show all issues

This should remain only in ReviewRequest.

reviewboard/reviews/models/base_review_request_details.py (Diff revision 12)
Show all issues
```
This should remain only in ReviewRequest.
```

reviewboard/reviews/models/base_review_request_details.py (Diff revision 12)

Show all issues

This should be a CharField, not a TextField. The latter is meant for larger pieces of text, and won't index as well.

We also want to move this to ReviewRequest and to ReviewRequestDraft. The difference between the two is that ReviewRequest's would have a default value for the Unpublished stage. This will apply to all brand new review requests (not existing ones from a legacy database). All review requests start out as Unpublished.

ReviewRequestDraft would not have a default value. The default would be set when the draft is created, based on the value on the review request. It's a copy, just like how we copy the summary, description, etc.

The fields also need:

null=True (so that we can have default, null values for existing database entries)
max_length=64 (to leave room for vendor prefixes for third-party extensions)

reviewboard/reviews/models/base_review_request_details.py (Diff revision 12)

Show all issues

Args is indented too far.

reviewboard/reviews/models/base_review_request_details.py (Diff revision 12)
Show all issues
```
`:py:class:
```

reviewboard/reviews/models/base_review_request_details.py (Diff revision 12)

Show all issues

This function should go away. All computation is going to happen in the controller.
ReviewRequest.__init__ will check if the stage is empty and, if so, call out to the controller to set a value.
ReviewRequestDraft.__init__ will do the same check, but set its own value from the review request.

reviewboard/reviews/models/base_review_request_details.py (Diff revision 12)
Show all issues
```
:py:class:
```

reviewboard/reviews/stages/__init__.py (Diff revision 12)

Show all issues

This file is more than just the registry. It's the whole module for working with stages. This specific file just happens to contain the registry.

It'd be nice to have this go into a little more detail on what stages are.

reviewboard/reviews/stages/base.py (Diff revision 12)

Show all issues

Read this as if you weren't aware of the stages work, and then see if you can think of some ways to flesh out the docstring.

reviewboard/reviews/stages/builtin_stages.py (Diff revision 12)
Show all issues
```
"built-in"
```

reviewboard/reviews/stages/builtin_stages.py (Diff revision 12)

Show all issues

No point in setting the values if they aren't changing from the base class. Can you go through each of these and make sure flags are only set if they differ in value?

reviewboard/reviews/stages/controller.py (Diff revision 12)

Show all issues

This should include some information on how those updates are applied, and how consumers might override computation.

reviewboard/reviews/stages/controller.py (Diff revision 12)

Show all issues

We usually say something like "Handler for review request published signals."

reviewboard/reviews/stages/controller.py (Diff revision 12)
Show all issues
```
This is the wrong module path.
Same below.
```

reviewboard/reviews/stages/controller.py (Diff revision 12)

Show all issues

You can combine these into one call:

self._update_review_request_stage(
    review_request,
    first_publish=changedesc is None)

reviewboard/reviews/stages/controller.py (Diff revision 12)
Show all issues
```
Should be (bool, optional)
```

reviewboard/reviews/stages/controller.py (Diff revision 12)

Show all issues

Let's use keyword arguments in the call. Helps for future expansion.

larmiej Dec. 10, 2016, 1:30 a.m.

just add **kwargs and should first_publish in the keyword arguments

reviewboard/reviews/stages/controller.py (Diff revision 12)
Show all issues
```
Blank line between these.
```
reviewboard/reviews/stages/controller.py (Diff revision 12)
Show all issues
```
:py:class:
```

reviewboard/reviews/stages/registry.py (Diff revision 12)

Show all issues

We don't use title case. This should better summarize what this class is. "A registry for tracking available stages for a review request."

reviewboard/reviews/stages/registry.py (Diff revision 12)

Show all issues

Missing period.
This should also flesh out the responsibilities of this class a bit more. Remember that, for every docstring, you should be trying to read this from the point of view of someone who has no idea what stages are, and the docstrings should educate on that.

reviewboard/reviews/stages/registry.py (Diff revision 12)

Show all issues

The docstring isn't correct. This isn't returning registered stages. It's returning the default stages that the registry knows about.

reviewboard/reviews/stages/registry.py (Diff revision 12)

Show all issues

A couple things here:

The description is redundant. That's implied by the type. Don't just talk return values and data types. Talk about purpose. What is actually being returned?
The data types are also wrong here. This is returning a tuple of default stages.

reviewboard/reviews/stages/registry.py (Diff revision 12)
Show all issues
```
:py:class:
```
reviewboard/reviews/stages/registry.py (Diff revision 12)
Show all issues
```
"... registered to the provided stage ID."
```

reviewboard/reviews/stages/registry.py (Diff revision 12)

Show all issues

As above, this should be talking about what's truly being returned beyond just a data type.

reviewboard/reviews/stages/tests.py (Diff revision 12)

Show all issues

A tests.py file should contain unit tests. Given the simplicity of this dummy object, I don't think we should bother keeping a One True Dummy Object. Instead, have each test suite define their own.

The reason for this is that, over time, complexity grows, and tests need to test more variations. You can run the risk of someone changing this object and accidentally impacting other tests.

If you have a One True Dummy Object, you're going to eventually need 50 True Dummy Objects, which will all end up shoved into the same place, and it just creates a maintenance nightmare.

So let's nuke this file and instead define a dummy stage in the tests/files that need them.

reviewboard/reviews/tests/test_stages.py (Diff revision 12)

Show all issues

Test docstrings should mention the classes/attribute/method/etc. being worked with. However, these tests are all testing different classes, which is often a red flag.

I think overall, this test suite is trying to do too many things. From the standpoint of your project, this all feels like "stage unit tests," but from the standpoint of the codebase, you're really testing different areas of responsibility.

What I'd suggest is:

Having any tests involving default review request stage state living in test_review_request.py.
Any tests involving review request draft state (copying initial values from the review request) should live in test_review_request_draft.py.
Tests involving the controller specifically should live in test_review_request_stage_controller.py.

reviewboard/reviews/tests/test_stages.py (Diff revision 12)

Show all issues

I wouldn't expect this call to be necessary? That should be calculated on access.

Change Summary:

modifying implementation so that stage controller handles all computational logic

Branch:

-	master
+	release-3.0.x

Commit:

-	b2309fc9d2a160ae16252ba0514eec15332f7728
+	172456c1321b67ad2df2eef9419d6dd9e4a16d80

Diff:

Revision 13 (+895 -1)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/datagrids/grids.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/reviews/__init__.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	12 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/tests/test_review_request_stage_controller.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/tests/test_review_request_draft.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/tests/test_stages.py
    reviewboard/extensions/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/tests/test_review_request_stage_controller.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/tests/test_review_request_draft.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/tests/test_stages.py
    reviewboard/extensions/tests.py

reviewboard/extensions/tests.py (Diff revision 13)
Show all issues
```
Col: 1
 W391 blank line at end of file
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 13)
Show all issues
```
 'DiscardedStage' imported but unused
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 13)
Show all issues
```
 'CompletedStage' imported but unused
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 13)
Show all issues
```
 'PendingReviewsStage' imported but unused
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 13)
Show all issues
```
 'UnpublishedStage' imported but unused
```
reviewboard/reviews/tests/test_review_request_stage_controller.py (Diff revision 13)
Show all issues
```
 'WIPStage' imported but unused
```
reviewboard/reviews/tests/test_review_request_stage_controller.py (Diff revision 13)
Show all issues
```
Col: 1
 W391 blank line at end of file
```

Change Summary:

deleting old test stages suite

Commit:

-	172456c1321b67ad2df2eef9419d6dd9e4a16d80
+	f0e7f2b194e1be0fab649571510fe23eb81f6243

Diff:

Revision 14 (+775 -1)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/datagrids/grids.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/reviews/__init__.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	11 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/tests/test_review_request_stage_controller.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/tests/test_review_request_draft.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/extensions/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/tests/test_review_request_stage_controller.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/tests/test_review_request_draft.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/extensions/tests.py

reviewboard/extensions/tests.py (Diff revision 14)
Show all issues
```
Col: 1
 W391 blank line at end of file
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 14)
Show all issues
```
 'DiscardedStage' imported but unused
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 14)
Show all issues
```
 'CompletedStage' imported but unused
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 14)
Show all issues
```
 'PendingReviewsStage' imported but unused
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 14)
Show all issues
```
 'UnpublishedStage' imported but unused
```
reviewboard/reviews/tests/test_review_request_stage_controller.py (Diff revision 14)
Show all issues
```
 'WIPStage' imported but unused
```
reviewboard/reviews/tests/test_review_request_stage_controller.py (Diff revision 14)
Show all issues
```
Col: 1
 W391 blank line at end of file
```

Change Summary:

fising bot issues

Commit:

-	f0e7f2b194e1be0fab649571510fe23eb81f6243
+	55d1ac517a3840532dea7ed0d6e57904b1492bc3

Diff:

Revision 15 (+767 -1)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/datagrids/grids.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/reviews/__init__.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	11 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/tests/test_review_request_stage_controller.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/tests/test_review_request_draft.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/extensions/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/tests/test_review_request_stage_controller.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/tests/test_review_request_draft.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/extensions/tests.py

Fix it!

reviewboard/reviews/__init__.py (Diff revision 15)

Show all issues

This should actually be None initially.

anni_cao Jan. 22, 2017, 9:50 a.m.

Tried to change to None but the code was broken with error "None object cannot call calculate_review_request_stage()"

reviewboard/reviews/models/review_request.py (Diff revision 15)

Show all issues

You need to do:

from reviewboard.reviews.stages import stage_controller

# ...

stage_controller.calculate_review_request_stage(self.get_review_request())

reviewboard/reviews/stages/controller.py (Diff revision 15)
Show all issues
```
This shouldn't be here.
```

Owner:

-	larmiej
+	anni_cao

Branch:

-	release-3.0.x
+	master

Commit:

-	55d1ac517a3840532dea7ed0d6e57904b1492bc3
+	394d3093e0bb56d10b55d9c045e7191f844df72b

Diff:

Revision 16 (+764 -1)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/datagrids/grids.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/reviews/__init__.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_stages.py
	11 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/tests/test_review_request_stage_controller.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/tests/test_review_request_draft.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/extensions/tests.py

reviewboard/extensions/tests.py (Diff revision 16)
Show all issues
```
 redefinition of unused 'TestCase' from line 55
```

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/stages/builtin_stages.py
    reviewboard/reviews/evolutions/add_stages.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/tests/test_review_request_stage_controller.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/__init__.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/reviews/stages/__init__.py
    reviewboard/reviews/stages/registry.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/stages/controller.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/datagrids/grids.py
    reviewboard/reviews/stages/base.py
    reviewboard/reviews/tests/test_review_request_draft.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/extensions/tests.py

Add Stage field to a review request

Information

Reviewers

Change Summary:

Diff:

Summary:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Testing Done:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Summary:

Change Summary:

Commit:

Diff:

Description:

Change Summary:

Description:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Branch:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Owner:

Branch:

Commit:

Diff:

Status: Discarded