Depends On & Blocks Relationships to Request Box.

Review Request #3881 — Created Feb. 17, 2013 and submitted

Information

Review Board
master
281

Reviewers

Feature
- This feature adds the ability to link requests together by
  means of two fields; depends on, and blocks. These relationships
  are interconnected in the sense that, if Request A depends on 
  Request B, then inversely Request B's blocks field will display
  a link back to Request A.
- This request adds the ability to add, edit, and remove dependencies
  from both the backend admin panel as well as in the front-end.
- Blocking relationships are built as a reverse of dependency, and
  thus are automatically generated upon a dependency change.
Backend
- Select from Review Request Dependency box a new request to depend on - Relationship has been added.
- Select from Review Request Draft Dependency box a new request to depend on - Relationship has been added.
- Viewed these changes on the front end - Blocks relationship has been adjusted in response for the selected request.

Frontend
- Clicked to change dependencies, added a proper request after a comma and space - properly checked and added to draft.
- Clicked to change dependencies, added an improper request after a comma and space - properly checked discarded with an accompanying warning.
- Clicked to change dependencies, added a proper request after a space - properly checked and added to draft.
- Clicked to change dependencies, added an improper request after a space - properly checked discarded with an accompanying warning.
- Removed a dependency - Changes remain removed.
- Published Changes - Changes remain added.
- Enter in one incorrect request vs. multiple incorrect requests - Error adjusts to pluralized form.
- Provide a change to an already published request - change log reports adjustment. 

Description From Last Updated

Since djblets is part of Django and reviewboard is part of Reviewboard, I think there should exists a blank line …

GR gregwym

I prefer "Do not change the things that are working, unless there is a good reason." This doesn't change anything …

GR gregwym

Same here, these blank lines are probably been inserted intentionally.

GR gregwym

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 67 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 67 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Looks like this is missing a space.

chipx86chipx86

Okay, so this is a good lesson in query optimization. Say we load 30 rows in the datagrid. What this …

chipx86chipx86

This looks... interesting :) What are you trying to do here?

chipx86chipx86

The other wrapping was more correct. If ReviewBot is yelling at you here, ignore it. Don't worry about fixing this …

chipx86chipx86

_() is for localizing human-readable strings. This is an identifier, which won't work so well if, say, a Russian translation …

chipx86chipx86

The indentation of all this is wrong. THey're off by one character.

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Same comment about not using _().

chipx86chipx86

Can you change these to use === instead of == ? (In JavaScript, == casts, and === doesn't.. the latter …

chipx86chipx86

Trailing commas will break many browsers.

chipx86chipx86

What's this all about?

chipx86chipx86

Note the other template tags. The {% should always be unindented, and the contents inside should be indented relative to …

chipx86chipx86

This looks wrong.

chipx86chipx86

These should explicitly say "review request". Same with the other field strings.

chipx86chipx86

This doesn't look right to me, and I feel very uncomfortable with changing old code like this that doesn't have …

chipx86chipx86

Some indentation weirdness. I would change this to start parameters on the second line, indented 4 spaces from the "obj". …

chipx86chipx86

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

reviewbotreviewbot

This shouldn't be here.

chipx86chipx86

"review request's" Also, trailing space should be removed.

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 39 E711 comparison to None should be 'if cond is None

reviewbotreviewbot

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

reviewbotreviewbot

I think this is worth some documentation.

mike_conleymike_conley

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

reviewbotreviewbot

This isn't something the others are doing yet, but you should use newer-style parent construction: super(DependsOnColumn, self).__init__(self, *args, **kwargs)

chipx86chipx86

No blank line here.

chipx86chipx86

I'm seeing some stuff like this that I've commented on on previous reviews, which were marked as fixed. Can you …

chipx86chipx86

If you're going to break this up like this, I think I'd prefer to be consistent about putting the space …

mike_conleymike_conley

Don't change things that aren't directly related to your change.

chipx86chipx86

Same.

chipx86chipx86

Even if pep8 complains, modifying this in your change only makes it harder for us to track down issues later. …

chipx86chipx86

Nope, put it back :)

chipx86chipx86

"by" should be wrapped with trans tags (although the ordering when translated might differ...by might come after...)... I assume you …

mike_conleymike_conley

ReviewBot might have a point. Maybe: obj = ReviewRequest.objects.get( Q(local_id=value) & Q(local_site=local_site))

mike_conleymike_conley

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

reviewbotreviewbot

Undo this change.

mike_conleymike_conley

Undo this change.

mike_conleymike_conley

Undo this change.

mike_conleymike_conley

Undo this change.

mike_conleymike_conley

Undo this change.

mike_conleymike_conley

Undo this change.

mike_conleymike_conley

Undo these changes.

mike_conleymike_conley

Undo this change.

mike_conleymike_conley

Undo this change.

mike_conleymike_conley

Undo this change.

mike_conleymike_conley

Undo this change.

mike_conleymike_conley

Undo this change.

mike_conleymike_conley

Undo this change.

mike_conleymike_conley

Undo these changes.

mike_conleymike_conley

Undo thsi change.

mike_conleymike_conley

Undo this change.

mike_conleymike_conley

Undo these changes.

mike_conleymike_conley

Undo this change.

mike_conleymike_conley

Undo this change.

mike_conleymike_conley

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

reviewbotreviewbot

Undo this change.

mike_conleymike_conley

related_name should never be localized. No _(..)

chipx86chipx86

Alignment issues. The parameters should be aligned to 'ReviewRequest'. The 'ReviewRequest' here should be able to be ReviewRequest (no quotes).

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Same here.

chipx86chipx86

These should really be ===. Can you change all three?

chipx86chipx86

This looks broken. I don't see how it can compile.

chipx86chipx86

Where did this go?

chipx86chipx86

===

chipx86chipx86

"Review request"

chipx86chipx86

===

chipx86chipx86

"Review requests"

chipx86chipx86

These should be unindented, but with the contents indented relative to the nearest template tags (like: {% foo %}). Same …

chipx86chipx86

class="" should be removed.

chipx86chipx86

"for this review request."

chipx86chipx86

"for this review request."

chipx86chipx86

"for this review request."

chipx86chipx86

We have this split in two places, and both are slightly different. One is "[, ]+" and one is "[,\s*]". …

chipx86chipx86

Rather than this, can you add a new function to ReviewRequestManager (reviews/managers.py) called, let's say, for_id(), that would contain this …

chipx86chipx86

pk=value

chipx86chipx86

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

reviewbotreviewbot

No blank line here.

chipx86chipx86

"review request's"

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

I believe verbose_name needs to be wrapped in _()

mike_conleymike_conley

Same here

mike_conleymike_conley

Template code should be indented within the `{% ... %}` label? So this might become, {% spaceless %} {% for …

GR gregwym

I'm not sure what this code is intending to do, but `,\s*` is quite different than `[,\s*]` in regex. `,\s*` …

GR gregwym

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

What happened to Christian's request? "Yeah, we should use the one we have in reviews/models.py." See the comments in my …

mike_conleymike_conley

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

reviewbotreviewbot

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

reviewbotreviewbot
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/datagrids.py
        reviewboard/reviews/admin.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. 
      
DE
DE
chipx86
  1. Did you mean to post this as a new review request? You already have one open.
    
    Make sure your description is always explaining the feature in detail, and not the changes you've made within your branch.
    1. I've edited it per your recommendations.
  2. 
      
DE
GR
  1. 
      
  2. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Since djblets is part of Django and reviewboard is part of Reviewboard, I think there should exists a blank line between their imports. 
  3. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
    I prefer "Do not change the things that are working, unless there is a good reason." 
    
    This doesn't change anything except the format, which has nothing to do with your patch's intension, but may introduce unnecessary merge conflict if someone has changed this part. 
    
    And there are many of others in this file. 
  4. reviewboard/reviews/views.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Same here, these blank lines are probably been inserted intentionally. 
    1. I agree with what your saying personally, especially about the formatting, however all these changes were from the PEP8 requirements, and proper formatting should be a part of every submission.
    2. Greg is correct about (most of) the blank lines between different imports. PEP-8 says to group imports into 3 sections--standard library, "third party", and "this package".
      
      In any case, it's better to split this up into two separate changes--one that fixes formatting of existing code, and one for your feature.
    3. I think, only the chunks of code we have changed are part of our submission, not the entire file. Basically when submitting a patch, a diff is everything. 
  5. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/datagrids.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/reviews/models.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  3. reviewboard/reviews/models.py (Diff revision 2)
     
     
    Col: 47
     E127 continuation line over-indented for visual indent
    
  4. reviewboard/reviews/models.py (Diff revision 2)
     
     
    Col: 47
     E127 continuation line over-indented for visual indent
    
  5. reviewboard/reviews/models.py (Diff revision 2)
     
     
    Col: 47
     E127 continuation line over-indented for visual indent
    
  6. reviewboard/reviews/models.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  7. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Col: 49
     E128 continuation line under-indented for visual indent
    
  8. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/datagrids.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/reviews/models.py (Diff revision 3)
     
     
    Col: 67
     W291 trailing whitespace
    
  3. reviewboard/reviews/models.py (Diff revision 3)
     
     
    Col: 47
     E127 continuation line over-indented for visual indent
    
  4. reviewboard/reviews/models.py (Diff revision 3)
     
     
    Col: 47
     E127 continuation line over-indented for visual indent
    
  5. reviewboard/reviews/models.py (Diff revision 3)
     
     
    Col: 47
     E127 continuation line over-indented for visual indent
    
  6. reviewboard/reviews/models.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  7. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  8. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Col: 58
     E127 continuation line over-indented for visual indent
    
  9. 
      
DE
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/datagrids.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/reviews/models.py (Diff revision 4)
     
     
    Col: 67
     W291 trailing whitespace
    
  3. reviewboard/webapi/resources.py (Diff revision 4)
     
     
    Col: 56
     E128 continuation line under-indented for visual indent
    
    1. There's no way to format this without having a PEP8 error.
  4. reviewboard/webapi/resources.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
    1. There's no way to format this without having a PEP8 error.
  5. reviewboard/webapi/resources.py (Diff revision 4)
     
     
    Col: 56
     E128 continuation line under-indented for visual indent
    
    1. There's no way to format this without having a PEP8 error.
  6. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/datagrids.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/reviews/models.py (Diff revision 5)
     
     
    Col: 40
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/reviews/models.py (Diff revision 5)
     
     
    Col: 40
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/reviews/models.py (Diff revision 5)
     
     
    Col: 40
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/webapi/resources.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  6. 
      
chipx86
  1. In general, this is shaping up nicely, but I have some comments. These fall primarily into two categories:
    
    1) Fixes you need to make to your change.
    2) Changes you need to revert.
    
    It's very important not to mix feature/bug fix changes and cleanup changes. Half this change are cleanup changes that have nothing to do with your work, and the cleanups aren't really things we want in some cases (ReviewBot isn't always 100% correct). I mention this below, but I'd ask that you undo every change in here that isn't related to your project. (Unfortunately, unless you did all that in their own commits, this might be a pain to do.)
  2. reviewboard/reviews/datagrids.py (Diff revision 5)
     
     
    Looks like this is missing a space.
    1. Dropped due to being in datagrid.
  3. reviewboard/reviews/datagrids.py (Diff revision 5)
     
     
    Okay, so this is a good lesson in query optimization. Say we load 30 rows in the datagrid. What this will do is perform this query 30 times, and bring in a lot of data with it. That's a huge increase in load.
    
    Instead, you want to augment the queryset. Look at the augment_queryset functions in that class for a bunch of different examples. Some of them are going to be more complex than others.
    
    What does the data look like that you're trying to show? Are these all summaries? If so, that's going to be very hard to read in a UI like the datagrid.
    1. Should I drop this one since it's related to the datagrid?
  4. reviewboard/reviews/datagrids.py (Diff revision 5)
     
     
    This looks... interesting :) What are you trying to do here?
  5. reviewboard/reviews/models.py (Diff revision 5)
     
     
     
     
     
    The other wrapping was more correct. If ReviewBot is yelling at you here, ignore it. Don't worry about fixing this and other fields unless you're working with them directly. It'll just make it harder for us to find the source of problems later when looking at who last touched the lines.
    
    Cleanup changes are fine, but don't merge them in with feature changes. I'd say, undo all that in this change. It'll also be easier for us, as we can focus on what actually changed in your code.
  6. reviewboard/reviews/models.py (Diff revision 5)
     
     
    _() is for localizing human-readable strings. This is an identifier, which won't work so well if, say, a Russian translation changes it.
  7. reviewboard/reviews/models.py (Diff revision 5)
     
     
     
     
     
    The indentation of all this is wrong. THey're off by one character.
  8. reviewboard/reviews/models.py (Diff revision 5)
     
     
    Same comment about not using _().
  9. reviewboard/static/rb/js/datastore.js (Diff revision 5)
     
     
     
     
    Can you change these to use === instead of == ? (In JavaScript, == casts, and === doesn't.. the latter is faster).
  10. reviewboard/static/rb/js/reviews.js (Diff revision 5)
     
     
    Trailing commas will break many browsers.
  11. What's this all about?
  12. Note the other template tags. The {% should always be unindented, and the contents inside should be indented relative to the other template tags.
  13. reviewboard/webapi/resources.py (Diff revision 5)
     
     
    These should explicitly say "review request". Same with the other field strings.
  14. reviewboard/webapi/resources.py (Diff revision 5)
     
     
    This doesn't look right to me, and I feel very uncomfortable with changing old code like this that doesn't have any bugs associated with it. What are you trying to do?
  15. reviewboard/webapi/resources.py (Diff revision 5)
     
     
     
     
    Some indentation weirdness. I would change this to start parameters on the second line, indented 4 spaces from the "obj".
    
    There's also some danger here with how you're doing this lookup. An ID is *very* different depending on whether it's connected to a Local Site or not. You'll need to query differently depending on whether local_site is None.
    1. I believe I've addressed this. I'll wait on confirmation before hitting Fixed.
  16. reviewboard/webapi/resources.py (Diff revision 5)
     
     
    This shouldn't be here.
  17. reviewboard/webapi/resources.py (Diff revision 5)
     
     
    "review request's"
    
    Also, trailing space should be removed.
  18. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/__init__.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/datagrids.py
        setup.py
      Ignored Files:
        docs/manual/webapi/2.0/resources/default-reviewer.txt
        docs/manual/webapi/2.0/resources/default-reviewer-list.txt
        docs/manual/webapi/2.0/resources/index.txt
        reviewboard/static/rb/js/datastore.js
        docs/releasenotes/reviewboard/index.txt
        reviewboard/cmdline/conf/reviewboard.wsgi.in
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/reviews.js
        docs/manual/docs.db
        docs/releasenotes/reviewboard/1.6.16.txt
        docs/releasenotes/reviewboard/1.7.6.txt
    
    
  2. reviewboard/reviews/models.py (Diff revision 6)
     
     
    Col: 40
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/reviews/models.py (Diff revision 6)
     
     
    Col: 40
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/reviews/models.py (Diff revision 6)
     
     
    Col: 40
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/webapi/resources.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  6. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/__init__.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/datagrids.py
        setup.py
      Ignored Files:
        docs/manual/webapi/2.0/resources/default-reviewer.txt
        docs/manual/webapi/2.0/resources/default-reviewer-list.txt
        docs/manual/webapi/2.0/resources/index.txt
        reviewboard/static/rb/js/datastore.js
        docs/releasenotes/reviewboard/index.txt
        reviewboard/cmdline/conf/reviewboard.wsgi.in
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/reviews.js
        docs/manual/docs.db
        docs/releasenotes/reviewboard/1.6.16.txt
        docs/releasenotes/reviewboard/1.7.6.txt
    
    
  2. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  3. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  4. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  5. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Col: 41
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Col: 41
     E126 continuation line over-indented for hanging indent
    
  7. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/__init__.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/datagrids.py
        setup.py
      Ignored Files:
        docs/manual/webapi/2.0/resources/default-reviewer.txt
        docs/manual/webapi/2.0/resources/default-reviewer-list.txt
        docs/manual/webapi/2.0/resources/index.txt
        reviewboard/static/rb/js/datastore.js
        docs/releasenotes/reviewboard/index.txt
        reviewboard/cmdline/conf/reviewboard.wsgi.in
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/reviews.js
        docs/manual/docs.db
        docs/releasenotes/reviewboard/1.6.16.txt
        docs/releasenotes/reviewboard/1.7.6.txt
    
    
  2. reviewboard/webapi/resources.py (Diff revision 8)
     
     
    Col: 41
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/webapi/resources.py (Diff revision 8)
     
     
    Col: 41
     E126 continuation line over-indented for hanging indent
    
  4. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/__init__.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/datagrids.py
        setup.py
      Ignored Files:
        docs/manual/webapi/2.0/resources/default-reviewer.txt
        docs/manual/webapi/2.0/resources/default-reviewer-list.txt
        docs/manual/webapi/2.0/resources/index.txt
        reviewboard/static/rb/js/datastore.js
        docs/releasenotes/reviewboard/index.txt
        reviewboard/cmdline/conf/reviewboard.wsgi.in
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/reviews.js
        docs/manual/docs.db
        docs/releasenotes/reviewboard/1.6.16.txt
        docs/releasenotes/reviewboard/1.7.6.txt
    
    
  2. reviewboard/webapi/resources.py (Diff revision 9)
     
     
    Col: 41
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/webapi/resources.py (Diff revision 9)
     
     
    Col: 41
     E126 continuation line over-indented for hanging indent
    
  4. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/__init__.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/datagrids.py
        setup.py
      Ignored Files:
        docs/manual/webapi/2.0/resources/default-reviewer.txt
        docs/manual/webapi/2.0/resources/default-reviewer-list.txt
        docs/manual/webapi/2.0/resources/index.txt
        reviewboard/static/rb/js/datastore.js
        docs/releasenotes/reviewboard/index.txt
        reviewboard/cmdline/conf/reviewboard.wsgi.in
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/reviews.js
        docs/manual/docs.db
        docs/releasenotes/reviewboard/1.6.16.txt
        docs/releasenotes/reviewboard/1.7.6.txt
    
    
  2. reviewboard/webapi/resources.py (Diff revision 10)
     
     
    Col: 39
     E711 comparison to None should be 'if cond is None
  3. reviewboard/webapi/resources.py (Diff revision 10)
     
     
    Col: 41
     E128 continuation line under-indented for visual indent
    
  4. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/__init__.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/datagrids.py
        setup.py
      Ignored Files:
        docs/manual/webapi/2.0/resources/default-reviewer.txt
        docs/manual/webapi/2.0/resources/default-reviewer-list.txt
        docs/manual/webapi/2.0/resources/index.txt
        reviewboard/static/rb/js/datastore.js
        docs/releasenotes/reviewboard/index.txt
        reviewboard/cmdline/conf/reviewboard.wsgi.in
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/reviews.js
        docs/manual/docs.db
        docs/releasenotes/reviewboard/1.6.16.txt
        docs/releasenotes/reviewboard/1.7.6.txt
    
    
  2. reviewboard/webapi/resources.py (Diff revision 11)
     
     
    Col: 41
     E128 continuation line under-indented for visual indent
    
  3. 
      
DE
mike_conley
  1. Hey Jon, it looks like there are fragments from some other changes in this review request. You didn't, I assume, make these changes to the Django version requirement and docs, for example.
    
    Can you please update this patch with just your changes?
  2. reviewboard/reviews/datagrids.py (Diff revision 11)
     
     
    I think this is worth some documentation.
  3. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/datagrids.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/webapi/resources.py (Diff revision 12)
     
     
    Col: 41
     E128 continuation line under-indented for visual indent
    
  3. 
      
mike_conley
  1. Hey Jon,
    
    This looks fantastic. Just some style nits from me, but I think this is looking really solid. One more review pass after these changes, and you've got my ship-it!
    
    -Mike
  2. reviewboard/reviews/datagrids.py (Diff revision 12)
     
     
    Is there supposed to be a space in between there? Or are we supposed to be trying to use as little space as possible here?
  3. reviewboard/reviews/models.py (Diff revision 12)
     
     
     
    If you're going to break this up like this, I think I'd prefer to be consistent about putting the space at the end of the line (so move the space from the beginning of 123 to the end of 122 if possible).
  4. "by" should be wrapped with trans tags (although the ordering when translated might differ...by might come after...)...
    
    I assume you talked this move over with ChipX86 or David?
  5. reviewboard/webapi/resources.py (Diff revision 12)
     
     
     
    ReviewBot might have a point. Maybe:
    
    obj = ReviewRequest.objects.get(
        Q(local_id=value) & Q(local_site=local_site))
  6. 
      
chipx86
  1. So a huge chunk of this change is still stuff that should not have been changed. I think we went over this once, but please go through this change and remove every single modification that is not directly related to your change.
    
    Yes, pep8 may complain about some things, but in some cases we want to ignore pep8's recommendations, and either way, such changes are best left to cleanup changes. As it is, too much time is being spent figuring out what in this change is general cleanup, and what's part of your feature.
    
    Please make that a priority and then let me know immediately when it's done so I can give another re-review.
    1. What's happened is I fixed these things, but we recently addressed an accidental master pull, and apparently the fix reverted all my changes.
    2. Doh! That sucks.
  2. reviewboard/reviews/datagrids.py (Diff revision 12)
     
     
    This isn't something the others are doing yet, but you should use newer-style parent construction:
    
    
        super(DependsOnColumn, self).__init__(self, *args, **kwargs)
  3. reviewboard/reviews/models.py (Diff revision 12)
     
     
    No blank line here.
  4. reviewboard/reviews/models.py (Diff revision 12)
     
     
     
     
     
    I'm seeing some stuff like this that I've commented on on previous reviews, which were marked as fixed. Can you go through my reviews and make sure they actually are?
  5. reviewboard/reviews/models.py (Diff revision 12)
     
     
     
     
    Don't change things that aren't directly related to your change.
  6. reviewboard/reviews/models.py (Diff revision 12)
     
     
     
     
  7. reviewboard/reviews/models.py (Diff revision 12)
     
     
     
    Even if pep8 complains, modifying this in your change only makes it harder for us to track down issues later. It's unrelated.
  8. reviewboard/reviews/models.py (Diff revision 12)
     
     
     
     
    Nope, put it back :)
  9. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/datagrids.py
        reviewboard/reviews/admin.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/webapi/resources.py (Diff revision 13)
     
     
    Col: 41
     E128 continuation line under-indented for visual indent
    
  3. 
      
mike_conley
  1. Jon,
    
    There's still a number of things that do not belong in this patch. I've opened issues for the ones I found.
    
    Otherwise, this is a really solid looking patch. I think I'll be pretty ready to give it a ship-it once these things are removed.
    
    -Mike
  2. reviewboard/reviews/admin.py (Diff revision 13)
     
     
    Undo this change.
  3. reviewboard/reviews/admin.py (Diff revision 13)
     
     
     
    Undo this change.
  4. reviewboard/reviews/models.py (Diff revision 13)
     
     
    Undo this change.
  5. reviewboard/reviews/models.py (Diff revision 13)
     
     
     
     
    Undo this change.
  6. reviewboard/reviews/models.py (Diff revision 13)
     
     
     
     
     
    Undo this change.
  7. reviewboard/reviews/models.py (Diff revision 13)
     
     
     
    Undo this change.
  8. reviewboard/reviews/models.py (Diff revision 13)
     
     
     
     
     
     
     
     
    Undo these changes.
  9. reviewboard/reviews/models.py (Diff revision 13)
     
     
    Undo this change.
  10. reviewboard/reviews/models.py (Diff revision 13)
     
     
     
    Undo this change.
  11. reviewboard/reviews/models.py (Diff revision 13)
     
     
    Undo this change.
  12. reviewboard/reviews/models.py (Diff revision 13)
     
     
    Undo this change.
  13. reviewboard/reviews/models.py (Diff revision 13)
     
     
    Undo this change.
  14. reviewboard/reviews/models.py (Diff revision 13)
     
     
     
     
    Undo this change.
  15. reviewboard/reviews/models.py (Diff revision 13)
     
     
     
     
     
     
     
     
     
    Undo these changes.
  16. reviewboard/reviews/models.py (Diff revision 13)
     
     
     
    Undo thsi change.
  17. reviewboard/reviews/models.py (Diff revision 13)
     
     
     
     
    Undo this change.
  18. reviewboard/reviews/models.py (Diff revision 13)
     
     
     
     
     
     
    Undo these changes.
  19. reviewboard/reviews/models.py (Diff revision 13)
     
     
     
    Undo this change.
  20. reviewboard/static/rb/js/datastore.js (Diff revision 13)
     
     
    Undo this change.
  21. reviewboard/webapi/resources.py (Diff revision 13)
     
     
     
     
    Undo this change.
    1. I've updated with the most recent diff. It both does not have the PEP8 mess up from before (what these are issues revolve around), as well as does not have the accidental merge that reverted my changes (thus bringing back the accidental PEP8 changes). I've also manually confirmed all these issues, so these are for sure no longer in the diff.
  22. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/reviews/models.py (Diff revision 14)
     
     
    Col: 40
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/reviews/models.py (Diff revision 14)
     
     
    Col: 40
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/reviews/models.py (Diff revision 14)
     
     
    Col: 40
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/webapi/resources.py (Diff revision 14)
     
     
    Col: 41
     E128 continuation line under-indented for visual indent
    
  6. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/webapi/resources.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  3. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/models.py (Diff revision 14)
     
     
    related_name should never be localized. No _(..)
  3. reviewboard/reviews/models.py (Diff revision 14)
     
     
     
     
     
    Alignment issues. The parameters should be aligned to 'ReviewRequest'.
    
    The 'ReviewRequest' here should be able to be ReviewRequest (no quotes).
  4. reviewboard/reviews/models.py (Diff revision 14)
     
     
    Same here.
  5. reviewboard/static/rb/js/datastore.js (Diff revision 14)
     
     
     
     
    These should really be ===. Can you change all three?
  6. reviewboard/static/rb/js/reviews.js (Diff revision 14)
     
     
     
     
    This looks broken. I don't see how it can compile.
    1. I'm going through these changes, and I'm noticing some inconsistancies between the actual diff (the one i'm looking at on my local copy/personal github) and what is being reported as changed here. This removal isn't actually in the code, same with a few of the previous comments. Confusing. :S
    2. Forgot to mention, these were fixed in one of the recent versions.
  7. reviewboard/static/rb/js/reviews.js (Diff revision 14)
     
     
    Where did this go?
  8. reviewboard/static/rb/js/reviews.js (Diff revision 14)
     
     
    "Review request"
  9. reviewboard/static/rb/js/reviews.js (Diff revision 14)
     
     
    "Review requests"
  10. These should be unindented, but with the contents indented relative to the nearest template tags (like: {%    foo %}).
    
    Same applies to all other added template tags in your change.
  11. class="" should be removed.
  12. reviewboard/webapi/resources.py (Diff revision 14)
     
     
    "for this review request."
  13. reviewboard/webapi/resources.py (Diff revision 14)
     
     
    "for this review request."
  14. reviewboard/webapi/resources.py (Diff revision 14)
     
     
    "for this review request."
  15. reviewboard/webapi/resources.py (Diff revision 14)
     
     
    We have this split in two places, and both are slightly different. One is "[, ]+" and one is "[,\s*]". I think we should consolidate these.
    
    I'd recommend adding a COMMA_SEPARATED_RE regex to BaseReviewRequestDetails (reviews/models.py) that is a compiled regex (re.compile(...)) and then reference that both here and in BaseReviewRequestDetails.
    1. I believe what happened here was that I saw the old way it was done, i.e. the original, and tried it out on a regex tester online to see how it worked (I'm new to REGEX), and by accident I pasted over the old way with the one I made in the regex tester just to see if it worked, and forgot to change it back. Should I just change it back?
    2. Yes, you should change this back if the functionality is the same.
    3. Yeah, we should use the one we have in reviews/models.py.
  16. reviewboard/webapi/resources.py (Diff revision 14)
     
     
     
     
     
     
    Rather than this, can you add a new function to ReviewRequestManager (reviews/managers.py) called, let's say, for_id(), that would contain this logic. That way it's in one place. Ideally, complex queries like this only exist in a single place in the codebase (we still have work to do to actually get there).
    1. So I've started to address this, but there seems to be something not clicking. 
      
      I've created a pastie to simply illustrate what I've done: http://pastie.org/7278394
      
      Basically, using the old ways, it works fine. i.e. RR #1 can add RR #2 as dependant, etc etc.
      
      However, upon the new changes, no matter what, the addition is always invalid, saying that the <insert entered number> doesn't exist. I'm pretty confused. Any thoughts?
    2. Hrm. I don't. I wonder if perhaps we should try fixing this in a follow-up...
    3. Any errors in the console? Sounds like something more is going on than there seems.
    4. It's the weirdest thing, it all now works as intended. Maybe my browser was wacky that day? Who knows, but it works perfect now with the new Manager change.
  17. reviewboard/webapi/resources.py (Diff revision 14)
     
     
    pk=value
  18. reviewboard/webapi/resources.py (Diff revision 14)
     
     
    No blank line here.
  19. reviewboard/webapi/resources.py (Diff revision 14)
     
     
    "review request's"
  20. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/webapi/resources.py (Diff revision 16)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  3. reviewboard/webapi/resources.py (Diff revision 16)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  4. reviewboard/webapi/resources.py (Diff revision 16)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  5. reviewboard/webapi/resources.py (Diff revision 16)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  6. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/webapi/resources.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  3. reviewboard/webapi/resources.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. 
      
mike_conley
  1. So there's Christian's one outstanding comment about the modified regex - that needs to be addressed, and then these below.
    
    Once those are fixed, I'm happy to give a ship it.
  2. reviewboard/reviews/models.py (Diff revision 17)
     
     
    I believe verbose_name needs to be wrapped in _()
  3. reviewboard/reviews/models.py (Diff revision 17)
     
     
    Same here
  4. 
      
GR
  1. Nice work! I can't wait for this patch be deployed on <http://reviews.reviewboard.org>! 
    
    The "text editor" patch is most likely going to depend on my "markdown" patch, so I might list your patch as my "markdown" patch's dependent! lmao...
  2. reviewboard/templates/reviews/review_request_box.html (Diff revision 17)
     
     
     
     
     
     
     
    Template code should be indented within the `{% ... %}` label? 
    
    So this might become, 
    
        {% spaceless %}
        {%  for dependency in review_request_details.depends_on.all %}
        ...
        {%   if not forloop.last %}, {% endif %}
        {%  endfor %}
        {% endspaceless %}
    
    1. Instead of just dropping this issue, can you explain why it was dropped Jon?
    2. Woops, I might've mixed this up, I don't know why this was closed. I'll change the formatting next.
      
    3. Fixed in the latest diff.
  3. reviewboard/webapi/resources.py (Diff revision 17)
     
     
    I'm not sure what this code is intending to do, but `,\s*` is quite different than `[,\s*]` in regex. 
    
    `,\s*` will match any string that start with a comma and follow by arbitrary number of space. 
    
    `[,\s*]` will match a character that is a comma, a space or a star(*). 
    
    
    When using these two regex to perform split, the result will be totally different. 
    
    For example, let `data = "abc,    def"`. 
    - `,\s*` will split the data into "abc" and "def"
    - `[,\s*]` will split the data into "abc", " ", " " and "def"
    
    If there is a star(*) within the string, it will become more messy. You can play around with regex on <http://regexpal.com/>
  4. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/managers.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/reviews/managers.py (Diff revision 18)
     
     
    Col: 45
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/webapi/resources.py (Diff revision 18)
     
     
    Col: 32
     E128 continuation line under-indented for visual indent
    
  4. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/managers.py
        reviewboard/reviews/models.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/admin.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/webapi/resources.py (Diff revision 19)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
mike_conley
  1. 
      
  2. reviewboard/webapi/resources.py (Diff revision 19)
     
     
    What happened to Christian's request?
    
    "Yeah, we should use the one we have in reviews/models.py."
    
    See the comments in my last review.
    1. I thought had to remove my accidental changes, and that it was policy not to affect things that I'm not introducing? Because the only reason this is even in this review request is because I messed with it to see how it works and I forgot to revert my changes. It has nothing to do with what I'm bringing with this submission (indirectly it does, as its in the same function as some of my changes, but its not a part of my changes).
    2. That being said, I might've misunderstood his comment. Is he saying to change it to: r"[, ]+", as found in BaseReviewRequestDetails for the get_bug_list function? Because that's the only split I can find in reviews/models.py
    3. Split with `[, ]+` should be a better practice. 
  3. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/managers.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/webapi/resources.py (Diff revision 20)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
chipx86
  1. Awesome job! Committing this. Only made a couple small fixes and added some unit tests. This will be part of 1.7.8.
  2. 
      
DE
Review request changed

Status: Closed (submitted)

Change Summary:

Made some small changes and pushed to release-1.7.x (a23c9e5)
Loading...