Add Stage field to a review request

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

Information

Review Board
master

Reviewers

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

    Missing trailing commma.

  3. Show all issues

    Single quotes.

  4. reviewboard/reviews/stages/registry.py (Diff revision 1)
     
     
    Show all issues

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

  5. 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
    
  6. reviewboard/reviews/stages/stage.py (Diff revision 1)
     
     
    Show all issues

    Move to builtin_stages.py

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

    MOve this to __init__.py

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

    This is unnecessary becuase of get_defaults

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

    Move to builtin_stages.py

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

    Move to builtin_stages.py

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

    Move to builtin_stages.py

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

    Move to builtin_stages.py

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

    Move to builtin_stages.py

  14. reviewboard/reviews/stages/stage.py (Diff revision 1)
     
     
    Show all issues

    Move to builtin_stages.py

  15. 
      
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)
     
     
     
     
    Show all issues

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

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

    Not sure why this is here.

  4. Show all issues

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

  5. Show all issues

    Docstring.

  6. reviewboard/reviews/stages/builtin_stages.py (Diff revision 2)
     
     
     
     
    Show all issues

    Dont need spacing between these. Same below.

  7. Show all issues

    Docstring for all these.

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

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

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

    Docstring.

  10. reviewboard/reviews/stages/stage.py (Diff revision 2)
     
     
    Show all issues

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

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

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

  4. reviewboard/reviews/__init__.py (Diff revision 4)
     
     
    Show all issues

    The first import should be from __future__ import unicode_literals.

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

    Blank line between these.

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

    """The global stage registry."""

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

    stages

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

  8. Show all issues

    This needs to explain the contents of the file.

  9. Show all issues

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

  10. Show all issues

    This should be localized:

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

    Same for all stages.

  11. Show all issues

    Wrong docstring.

  12. Show all issues

    Please use proper capitilization and punctuation.

  13. Show all issues

    Docstring.

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

  15. Show all issues

    _on_review_request_published.

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

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

    This needs to explain the contents of the file.

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

    Can you explain what this is.

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

    Can you add a get_stage(stage_id) method?

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

    Missing docstring.

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

    You can just return a list here.

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

    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)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. Show all issues
    Col: 45
     E127 continuation line over-indented for visual indent
    
  4. Show all issues
    Col: 29
     E126 continuation line over-indented for hanging indent
    
  5. reviewboard/reviews/stages/registry.py (Diff revision 5)
     
     
    Show all issues
    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)
     
     
    Show all issues
    Col: 80
     E501 line too long (95 > 79 characters)
    
  3. 
      
brennie
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 6)
     
     
    Show all issues

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

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

    missing **kwargs in Args:

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

    Missing file level docstring.

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

    Docstring.

  6. Show all issues

    Missing docstring.

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

  8. Show all issues

    These all need translation.

  9. Show all issues

    missing a space

  10. Show all issues

    "completed"

  11. Show all issues

    Docstrings here and elsewhere.

  12. Show all issues

    No blank line

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

  14. Show all issues

    ('_stage',)

    Using a tuple requires less memory allocation.

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

  16. reviewboard/reviews/stages/registry.py (Diff revision 6)
     
     
     
     
     
     
     
     
    Show all issues

    You can just return this instead of yielding.

  17. 
      
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. Show all issues

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

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

    Use the full import path.

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

    Blank line between these.

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

    django.utils.safestring.SafeText

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

    No blank line.

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

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

    No blank line.

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

    Two blank lines.

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

    Blank line.

  11. Show all issues

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

  12. Show all issues

    Docstrings for all these.

  13. Show all issues

    This is going to need many unit tests.

  14. Show all issues

    No blank line.

  15. Show all issues

    if not new_stage

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

  17. Show all issues

    No blank line.

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

    "for review requests."

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

    Missing Returns.

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

    Just return the tuple.

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

  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)
     
     
    Show all issues
    Col: 27
     E128 continuation line under-indented for visual indent
    
  3. Show all issues
    Col: 80
     E501 line too long (117 > 79 characters)
    
  4. Show all issues
    Col: 80
     E501 line too long (116 > 79 characters)
    
  5. Show all issues
    Col: 80
     E501 line too long (121 > 79 characters)
    
  6. Show all issues
    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. Show all issues

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    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)
     
     
    Show all issues

    Should end in a period.

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

    Description should go on the next line.

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

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

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

    Needs a period.

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

  7. Show all issues

    Can we alphabetize these?

  8. Show all issues

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

  9. Show all issues

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

  10. Show all issues

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

  11. Show all issues

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

  12. Show all issues

    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. Show all issues

    Same here.

  14. Show all issues

    Same here.

  15. reviewboard/reviews/stages/__init__.py (Diff revision 9)
     
     
    Show all issues

    Add a period.

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

  17. Show all issues

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

  18. Show all issues

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

  19. Show all issues

    Description should go on the next line.

  20. Show all issues

    Description should go on the next line.

  21. Show all issues

    Description should go on the next line.

  22. Show all issues

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

  23. Show all issues

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

  24. Show all issues

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

  25. 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)):
    
  26. reviewboard/reviews/stages/registry.py (Diff revision 9)
     
     
     
     
     
     
     
     
    Show all issues

    Alphabetize?

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

  28. reviewboard/reviews/stages/registry.py (Diff revision 9)
     
     
    Show all issues

    Same here. Also needs to end in a period.

  29. reviewboard/reviews/stages/tests.py (Diff revision 9)
     
     
    Show all issues

    Needs a period.

  30. reviewboard/reviews/tests/test_stages.py (Diff revision 9)
     
     
     
     
     
     
     
     
    Show all issues

    Alphabetize?

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

  32. 
      
brennie
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 9)
     
     
    Show all issues

    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)
     
     
    Show all issues

    Should be two blank lines here.

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

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

    Mind adding another blank line here?

  5. Show all issues

    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)
     
     
     
     
     
     
    Show all issues

    We should move this to BaseReviewRequestDetails too.

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

    Add a blank line here.

  8. Show all issues

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

  9. Show all issues

    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)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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)
     
     
     
     
     
     
     
    Show all issues

    Here too.

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

    And here, and below.

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

    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)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues

    Missing docs on Args.

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

    No indentation here for the description.

  4. 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:
        ...
    
  5. reviewboard/datagrids/grids.py (Diff revision 12)
     
     
    Show all issues

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

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

    Should be reviewboard.extensions.base.Extension.

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

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

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

  10. Show all issues

    Needs a trailing comma.

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

    This should remain only in ReviewRequest.

  12. Show all issues

    This should remain only in ReviewRequest.

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

    Args is indented too far.

  15. Show all issues

    `:py:class:

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

  17. Show all issues

    :py:class:

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

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

  20. Show all issues

    "built-in"

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

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

  23. reviewboard/reviews/stages/controller.py (Diff revision 12)
     
     
    Show all issues

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

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

    This is the wrong module path.

    Same below.

  25. 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)
    
  26. reviewboard/reviews/stages/controller.py (Diff revision 12)
     
     
    Show all issues

    Should be (bool, optional)

  27. reviewboard/reviews/stages/controller.py (Diff revision 12)
     
     
     
    Show all issues

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

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

  28. reviewboard/reviews/stages/controller.py (Diff revision 12)
     
     
     
    Show all issues

    Blank line between these.

  29. reviewboard/reviews/stages/controller.py (Diff revision 12)
     
     
    Show all issues

    :py:class:

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

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

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

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

    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.
  34. reviewboard/reviews/stages/registry.py (Diff revision 12)
     
     
    Show all issues

    :py:class:

  35. reviewboard/reviews/stages/registry.py (Diff revision 12)
     
     
    Show all issues

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

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

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

  38. 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:

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

  40. 
      
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)
     
     
    Show all issues
    Col: 1
     W391 blank line at end of file
    
  3. Show all issues
     'DiscardedStage' imported but unused
    
  4. Show all issues
     'CompletedStage' imported but unused
    
  5. Show all issues
     'PendingReviewsStage' imported but unused
    
  6. Show all issues
     'UnpublishedStage' imported but unused
    
  7. Show all issues
     'WIPStage' imported but unused
    
  8. Show all issues
    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)
     
     
    Show all issues
    Col: 1
     W391 blank line at end of file
    
  3. Show all issues
     'DiscardedStage' imported but unused
    
  4. Show all issues
     'CompletedStage' imported but unused
    
  5. Show all issues
     'PendingReviewsStage' imported but unused
    
  6. Show all issues
     'UnpublishedStage' imported but unused
    
  7. Show all issues
     'WIPStage' imported but unused
    
  8. Show all issues
    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)
     
     
    Show all issues

    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. Show all issues

    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)
     
     
    Show all issues

    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)
     
     
    Show all issues
     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