Add Stage field to a review request

Review Request #8495 — Created Oct. 26, 2016 and discarded

anni_cao
Review Board
master
reviewboard, students

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

All tests pass

Description From Last Updated

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

brenniebrennie

Missing trailing commma.

brenniebrennie

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

Single quotes.

brenniebrennie

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

brenniebrennie

This should be formatted like a docstring, i.e. #: Single line summary #: #: Multi-line description should_review = False

brenniebrennie

Move to builtin_stages.py

brenniebrennie

MOve this to __init__.py

brenniebrennie

This is unnecessary becuase of get_defaults

brenniebrennie

Move to builtin_stages.py

brenniebrennie

Move to builtin_stages.py

brenniebrennie

Move to builtin_stages.py

brenniebrennie

Move to builtin_stages.py

brenniebrennie

Move to builtin_stages.py

brenniebrennie

Move to builtin_stages.py

brenniebrennie

Two blank lines between these. Missing a file docstring. Missing from __future__ import unicode literals.

brenniebrennie

Not sure why this is here.

brenniebrennie

Misisng file-level docstring. Missing from __future__ import unicode literals.

brenniebrennie

Docstring.

brenniebrennie

Dont need spacing between these. Same below.

brenniebrennie

Docstring for all these.

brenniebrennie

Misisng file-level docstring. Missing from __future__ import unicode literals.

brenniebrennie

Docstring.

brenniebrennie

Misisng file-level docstring. Missing from __future__ import unicode literals.

brenniebrennie

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Args/kwargs missing.

brenniebrennie

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

brenniebrennie

The first import should be from __future__ import unicode_literals.

brenniebrennie

Blank line between these.

brenniebrennie

"""The global stage registry."""

brenniebrennie

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

brenniebrennie

This needs to explain the contents of the file.

brenniebrennie

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

brenniebrennie

This should be localized: from django.utils.translation import ugettext_lazy as _ # ... class UnpublishedStage(BaseReviewRequestStage): name = _('Unpublished') # ... Same ...

brenniebrennie

Wrong docstring.

brenniebrennie

Please use proper capitilization and punctuation.

brenniebrennie

Docstring.

brenniebrennie

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), ...

brenniebrennie

_on_review_request_published.

brenniebrennie

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

brenniebrennie

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

reviewbotreviewbot

This needs to explain the contents of the file.

brenniebrennie

Can you explain what this is.

brenniebrennie

Can you add a get_stage(stage_id) method?

brenniebrennie

Missing docstring.

brenniebrennie

You can just return a list here.

brenniebrennie

This needs to explain the contents of the file.

brenniebrennie

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

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

brenniebrennie

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

reviewbotreviewbot

missing **kwargs in Args:

brenniebrennie

Missing file level docstring.

brenniebrennie

Docstring.

brenniebrennie

Missing docstring.

brenniebrennie

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

brenniebrennie

These all need translation.

brenniebrennie

missing a space

brenniebrennie

"completed"

brenniebrennie

Docstrings here and elsewhere.

brenniebrennie

No blank line

brenniebrennie

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

brenniebrennie

('_stage',) Using a tuple requires less memory allocation.

brenniebrennie

Missing period. If this is a generator this should be "Yield the registered stage classes." If this is not it ...

brenniebrennie

You can just return this instead of yielding.

brenniebrennie

Use the full import path.

brenniebrennie

Blank line between these.

brenniebrennie

django.utils.safestring.SafeText

brenniebrennie

No blank line.

brenniebrennie

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

brenniebrennie

No blank line.

brenniebrennie

Two blank lines.

brenniebrennie

Blank line.

brenniebrennie

This should be reviewboard.reviews.stages.stages.BaseReviewRequestStage.

brenniebrennie

Docstrings for all these.

brenniebrennie

This is going to need many unit tests.

brenniebrennie

No blank line.

brenniebrennie

if not new_stage

brenniebrennie

Should be :py:class:`.... Also the :py:class bit needs to be on the same line as the class bit. It is ...

brenniebrennie

No blank line.

brenniebrennie

"for review requests."

brenniebrennie

Missing Returns.

brenniebrennie

Just return the tuple.

brenniebrennie

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

brenniebrennie

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 E303 too many blank lines (3)

reviewbotreviewbot

Missing a period. Docstrings should be complete sentences.

brenniebrennie

Should end in a period.

daviddavid

Description should go on the next line.

daviddavid

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

daviddavid

Needs a period.

daviddavid

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

daviddavid

Can we alphabetize these?

daviddavid

Should be the imperative mood ("Return ..." instead of "Returns ...")

daviddavid

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

daviddavid

Should be capitalized and imperative mood ("Set" instead of "sets")

daviddavid

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

daviddavid

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

daviddavid

Same here.

daviddavid

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

ST stensby

Same here.

daviddavid

Add a period.

daviddavid

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

ST stensby

This description is a little circular. How about "The Review Request has not yet been published"? Also, do we want ...

daviddavid

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

daviddavid

Maybe "Waiting for the author to make requested changes"?

daviddavid

Description should go on the next line.

daviddavid

Description should go on the next line.

daviddavid

Description should go on the next line.

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

Instead of using the character to continue the statement, please wrap the whole conditional in parentheses. We can also use ...

daviddavid

Alphabetize?

daviddavid

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

daviddavid

Same here. Also needs to end in a period.

daviddavid

Needs a period.

daviddavid

Alphabetize?

daviddavid

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

daviddavid

Should be two blank lines here.

daviddavid

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

daviddavid

Mind adding another blank line here?

daviddavid

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

daviddavid

We should move this to BaseReviewRequestDetails too.

daviddavid

Add a blank line here.

daviddavid

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

daviddavid

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

daviddavid

Dedent these 4 spaces.

daviddavid

Here too.

daviddavid

And here, and below.

daviddavid

review_request.stage should be vertically aligned with first_publish.

daviddavid

These can be one statement (just return the tuple)

daviddavid

Missing docs on Args.

chipx86chipx86

No indentation here for the description.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Should be reviewboard.extensions.base.Extension.

chipx86chipx86

There can't be a blank line here. Also, it must be :py:class:, not py:class:

chipx86chipx86

Docstrings are only valuable when they convey something the user doesn't already know. This just says what __init__.py is, generally. ...

chipx86chipx86

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

chipx86chipx86

Needs a trailing comma.

chipx86chipx86

This should remain only in ReviewRequest.

chipx86chipx86

This should remain only in ReviewRequest.

chipx86chipx86

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

chipx86chipx86

Args is indented too far.

chipx86chipx86

`:py:class:

chipx86chipx86

This function should go away. All computation is going to happen in the controller. ReviewRequest.__init__ will check if the stage ...

chipx86chipx86

:py:class:

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

"built-in"

chipx86chipx86

No point in setting the values if they aren't changing from the base class. Can you go through each of ...

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This is the wrong module path. Same below.

chipx86chipx86

You can combine these into one call: self._update_review_request_stage( review_request, first_publish=changedesc is None)

chipx86chipx86

Should be (bool, optional)

chipx86chipx86

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

chipx86chipx86

Blank line between these.

chipx86chipx86

:py:class:

chipx86chipx86

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

chipx86chipx86

Missing period. This should also flesh out the responsibilities of this class a bit more. Remember that, for every docstring, ...

chipx86chipx86

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

chipx86chipx86

A couple things here: The description is redundant. That's implied by the type. Don't just talk return values and data ...

chipx86chipx86

:py:class:

chipx86chipx86

"... registered to the provided stage ID."

chipx86chipx86

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

chipx86chipx86

A tests.py file should contain unit tests. Given the simplicity of this dummy object, I don't think we should bother ...

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Col: 1 W391 blank line at end of file

reviewbotreviewbot

'DiscardedStage' imported but unused

reviewbotreviewbot

'CompletedStage' imported but unused

reviewbotreviewbot

'PendingReviewsStage' imported but unused

reviewbotreviewbot

'UnpublishedStage' imported but unused

reviewbotreviewbot

'WIPStage' imported but unused

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

'DiscardedStage' imported but unused

reviewbotreviewbot

'CompletedStage' imported but unused

reviewbotreviewbot

'PendingReviewsStage' imported but unused

reviewbotreviewbot

'UnpublishedStage' imported but unused

reviewbotreviewbot

'WIPStage' imported but unused

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

This should actually be None initially.

brenniebrennie

You need to do: from reviewboard.reviews.stages import stage_controller # ... stage_controller.calculate_review_request_stage(self.get_review_request())

brenniebrennie

This shouldn't be here.

brenniebrennie

redefinition of unused 'TestCase' from line 55

reviewbotreviewbot
reviewbot
  1. 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
    
    
  2. Col: 80
     E501 line too long (85 > 79 characters)
    
  3. Col: 80
     E501 line too long (89 > 79 characters)
    
  4. Col: 1
     W391 blank line at end of file
    
  5. 
      
brennie
  1. 
      
  2. Missing trailing commma.

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

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

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

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

    #: Single line summary
    #:
    #: Multi-line description
    should_review = False
    
  5. reviewboard/reviews/stages/stage.py (Diff revision 1)
     
     

    Move to builtin_stages.py

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

    MOve this to __init__.py

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

    This is unnecessary becuase of get_defaults

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

    Move to builtin_stages.py

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

    Move to builtin_stages.py

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

    Move to builtin_stages.py

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

    Move to builtin_stages.py

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

    Move to builtin_stages.py

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

    Move to builtin_stages.py

  14. 
      
LA
LA
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/reviews/stages/__init__.py (Diff revision 2)
     
     
     
     

    Two blank lines between these.
    Missing a file docstring.
    Missing from __future__ import unicode literals.

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

    Not sure why this is here.

  4. Misisng file-level docstring.
    Missing from __future__ import unicode literals.

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

    Dont need spacing between these. Same below.

  6. Docstring for all these.

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

    Misisng file-level docstring.
    Missing from __future__ import unicode literals.

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

    Docstring.

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

    Misisng file-level docstring.
    Missing from __future__ import unicode literals.

  10. 
      
LA
reviewbot
  1. 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
    
    
  2. Col: 38
     E128 continuation line under-indented for visual indent
    
  3. Col: 80
     E501 line too long (80 > 79 characters)
    
  4. Col: 80
     E501 line too long (87 > 79 characters)
    
  5. 
      
LA
reviewbot
  1. 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
    
    
  2. Col: 38
     E128 continuation line under-indented for visual indent
    
  3. 
      
brennie
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 4)
     
     

    Args/kwargs missing.

    1. I don't understand what you mean, should I add in the args into the comment?

    2. Yes.

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

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

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

    The first import should be from __future__ import unicode_literals.

  5. reviewboard/reviews/builtin_fields.py (Diff revision 4)
     
     
     

    Blank line between these.

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

    """The global stage registry."""

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

    stages

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

  8. This needs to explain the contents of the file.

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

  10. This should be localized:

    from django.utils.translation import ugettext_lazy as _
    
    
    # ...
    
    class UnpublishedStage(BaseReviewRequestStage):
        name = _('Unpublished')
        # ...
    

    Same for all stages.

  11. Wrong docstring.

  12. Please use proper capitilization and punctuation.

  13. Docstring.

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

    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)

  15. _on_review_request_published.

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

    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.

  17. reviewboard/reviews/stages/registry.py (Diff revision 4)
     
     

    This needs to explain the contents of the file.

  18. reviewboard/reviews/stages/registry.py (Diff revision 4)
     
     

    Can you explain what this is.

  19. reviewboard/reviews/stages/registry.py (Diff revision 4)
     
     

    Can you add a get_stage(stage_id) method?

  20. reviewboard/reviews/stages/registry.py (Diff revision 4)
     
     

    Missing docstring.

  21. reviewboard/reviews/stages/registry.py (Diff revision 4)
     
     
     
     
     
     
     
     

    You can just return a list here.

  22. reviewboard/reviews/stages/stage.py (Diff revision 4)
     
     

    This needs to explain the contents of the file.

  23. 
      
LA
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/hooks.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. Col: 45
     E127 continuation line over-indented for visual indent
    
  4. Col: 29
     E126 continuation line over-indented for hanging indent
    
  5. reviewboard/reviews/stages/registry.py (Diff revision 5)
     
     
    Col: 1
     W391 blank line at end of file
    
  6. 
      
LA
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/hooks.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (95 > 79 characters)
    
  3. 
      
brennie
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 6)
     
     

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

  3. reviewboard/extensions/hooks.py (Diff revision 6)
     
     

    missing **kwargs in Args:

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

    Missing file level docstring.

  5. reviewboard/reviews/__init__.py (Diff revision 6)
     
     

    Docstring.

  6. Missing docstring.

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

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

  8. These all need translation.

  9. missing a space

  10. Docstrings here and elsewhere.

  11. No blank line

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

    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.

  13. ('_stage',)

    Using a tuple requires less memory allocation.

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

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

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

    You can just return this instead of yielding.

  16. 
      
LA
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. This is looking pretty solid. You will probably want to move on to writing unit tests.

  3. reviewboard/datagrids/columns.py (Diff revision 7)
     
     

    Use the full import path.

  4. reviewboard/datagrids/columns.py (Diff revision 7)
     
     
     

    Blank line between these.

  5. reviewboard/datagrids/columns.py (Diff revision 7)
     
     

    django.utils.safestring.SafeText

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

    No blank line.

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

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

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

    No blank line.

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

    Two blank lines.

  10. reviewboard/reviews/builtin_fields.py (Diff revision 7)
     
     
     

    Blank line.

  11. This should be reviewboard.reviews.stages.stages.BaseReviewRequestStage.

  12. Docstrings for all these.

  13. This is going to need many unit tests.

  14. No blank line.

  15. if not new_stage

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

    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.

  17. No blank line.

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

    "for review requests."

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

    Missing Returns.

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

    Just return the tuple.

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

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

  22. 
      
LA
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/tests.py (Diff revision 8)
     
     
    Col: 27
     E128 continuation line under-indented for visual indent
    
  3. Col: 80
     E501 line too long (117 > 79 characters)
    
  4. Col: 80
     E501 line too long (116 > 79 characters)
    
  5. Col: 80
     E501 line too long (121 > 79 characters)
    
  6. Col: 1
     E303 too many blank lines (3)
    
  7. 
      
LA
reviewbot
  1. 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
    
    
  2. 
      
LA
ST
  1. 
      
  2. Can review_request.status be something other than S, D and P?

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

  3. reviewboard/reviews/stages/base.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Do you need this many comments for the variables?  Most of them seem very self-explanatory.
    1. yeah, these comments were originally detailed as such in the orignal class design. And more information does not hurt

  4. 
      
david
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 9)
     
     

    Should end in a period.

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

    Description should go on the next line.

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

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

  5. reviewboard/reviews/__init__.py (Diff revision 9)
     
     

    Needs a period.

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

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

  7. Can we alphabetize these?

  8. Should be the imperative mood ("Return ..." instead of "Returns ...")

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

  10. Should be capitalized and imperative mood ("Set" instead of "sets")

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

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

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

  13. reviewboard/reviews/stages/__init__.py (Diff revision 9)
     
     

    Add a period.

  14. 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?

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

  16. Maybe "Waiting for the author to make requested changes"?

  17. Description should go on the next line.

  18. Description should go on the next line.

  19. Description should go on the next line.

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

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

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

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

    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)):
    
  24. reviewboard/reviews/stages/registry.py (Diff revision 9)
     
     
     
     
     
     
     
     

    Alphabetize?

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

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

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

    Same here. Also needs to end in a period.

  27. reviewboard/reviews/stages/tests.py (Diff revision 9)
     
     

    Needs a period.

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

    Alphabetize?

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

  30. 
      
brennie
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 9)
     
     

    Missing a period. Docstrings should be complete sentences.

    1. Meant to discard this review but published accidentally.

  3. 
      
LA
reviewbot
  1. 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
    
    
  2. 
      
LA
LA
LA
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/extensions/hooks.py (Diff revision 11)
     
     

    Should be two blank lines here.

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

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

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

    Mind adding another blank line here?

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

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

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

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

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

    We should move this to BaseReviewRequestDetails too.

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

    Add a blank line here.

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

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

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

    Dedent these 4 spaces.

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

    2. 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.
      """
      
  11. reviewboard/reviews/stages/controller.py (Diff revision 11)
     
     
     
     
     
     
     

    Here too.

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

    And here, and below.

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

    review_request.stage should be vertically aligned with first_publish.

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

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

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

    These can be one statement (just return the tuple)

  15. 
      
LA
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 12)
     
     

    Missing docs on Args.

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

    No indentation here for the description.

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

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

    if review_request.stage:
        ...
    
  5. reviewboard/datagrids/grids.py (Diff revision 12)
     
     

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

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

    Should be reviewboard.extensions.base.Extension.

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

    There can't be a blank line here.

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

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

    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.

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

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

  10. Needs a trailing comma.

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

    This should remain only in ReviewRequest.

  12. This should remain only in ReviewRequest.

  13. 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)
  14. reviewboard/reviews/models/base_review_request_details.py (Diff revision 12)
     
     
     
     
     
     
     
     
     

    Args is indented too far.

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

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

    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.

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

    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.

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

    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?

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

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

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

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

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

    This is the wrong module path.

    Same below.

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

    You can combine these into one call:

    self._update_review_request_stage(
        review_request,
        first_publish=changedesc is None)
    
  23. reviewboard/reviews/stages/controller.py (Diff revision 12)
     
     

    Should be (bool, optional)

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

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

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

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

    Blank line between these.

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

    :py:class:

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

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

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

    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.

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

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

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

    A couple things here:

    1. 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?
    2. The data types are also wrong here. This is returning a tuple of default stages.
  31. reviewboard/reviews/stages/registry.py (Diff revision 12)
     
     

    :py:class:

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

    "... registered to the provided stage ID."

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

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

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

    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.

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

    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:

    1. Having any tests involving default review request stage state living in test_review_request.py.
    2. Any tests involving review request draft state (copying initial values from the review request) should live in test_review_request_draft.py.
    3. Tests involving the controller specifically should live in test_review_request_stage_controller.py.
  36. reviewboard/reviews/tests/test_stages.py (Diff revision 12)
     
     

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

  37. 
      
LA
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/tests.py (Diff revision 13)
     
     
    Col: 1
     W391 blank line at end of file
    
  3.  'DiscardedStage' imported but unused
    
  4.  'CompletedStage' imported but unused
    
  5.  'PendingReviewsStage' imported but unused
    
  6.  'UnpublishedStage' imported but unused
    
  7.  'WIPStage' imported but unused
    
  8. Col: 1
     W391 blank line at end of file
    
  9. 
      
LA
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/tests.py (Diff revision 14)
     
     
    Col: 1
     W391 blank line at end of file
    
  3.  'DiscardedStage' imported but unused
    
  4.  'CompletedStage' imported but unused
    
  5.  'PendingReviewsStage' imported but unused
    
  6.  'UnpublishedStage' imported but unused
    
  7.  'WIPStage' imported but unused
    
  8. Col: 1
     W391 blank line at end of file
    
  9. 
      
LA
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/reviews/__init__.py (Diff revision 15)
     
     

    This should actually be None initially.

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

  3. You need to do:

    from reviewboard.reviews.stages import stage_controller
    
    # ...
    
    stage_controller.calculate_review_request_stage(self.get_review_request())
    
  4. reviewboard/reviews/stages/controller.py (Diff revision 15)
     
     

    This shouldn't be here.

  5. 
      
brennie
AN
reviewbot
  1. 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
    
    
  2. reviewboard/extensions/tests.py (Diff revision 16)
     
     
     redefinition of unused 'TestCase' from line 55
    
  3. 
      
reviewbot
  1. 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
    
    
  2. 
      
david
Review request changed

Status: Discarded

Loading...