- Summary:
-
*{WIP]* - Redone Dependencies: Request <-> Request AND Draft Request -> Request FROM Admin Panel*[WIP]* - Redone Dependencies: Request <-> Request AND Draft Request -> Request FROM Admin Panel
Depends On & Blocks Relationships to Request Box.
Review Request #3881 — Created Feb. 17, 2013 and submitted
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) |
 reviewbot
|
|
|
Col: 47 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Col: 47 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Col: 47 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
Col: 49 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 67 W291 trailing whitespace |
reviewbot
|
|
|
Col: 47 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Col: 47 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Col: 47 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
Col: 58 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Col: 67 W291 trailing whitespace |
reviewbot
|
|
|
Col: 56 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 56 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Looks like this is missing a space. |
 chipx86
|
|
|
Okay, so this is a good lesson in query optimization. Say we load 30 rows in the datagrid. What this … |
chipx86
|
|
|
This looks... interesting :) What are you trying to do here? |
chipx86
|
|
|
The other wrapping was more correct. If ReviewBot is yelling at you here, ignore it. Don't worry about fixing this … |
chipx86
|
|
|
_() is for localizing human-readable strings. This is an identifier, which won't work so well if, say, a Russian translation … |
chipx86
|
|
|
The indentation of all this is wrong. THey're off by one character. |
chipx86
|
|
|
Col: 40 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 40 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 40 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Same comment about not using _(). |
chipx86
|
|
|
Can you change these to use === instead of == ? (In JavaScript, == casts, and === doesn't.. the latter … |
chipx86
|
|
|
Trailing commas will break many browsers. |
chipx86
|
|
|
What's this all about? |
chipx86
|
|
|
Note the other template tags. The {% should always be unindented, and the contents inside should be indented relative to … |
chipx86
|
|
|
This looks wrong. |
chipx86
|
|
|
These should explicitly say "review request". Same with the other field strings. |
chipx86
|
|
|
This doesn't look right to me, and I feel very uncomfortable with changing old code like this that doesn't have … |
chipx86
|
|
|
Some indentation weirdness. I would change this to start parameters on the second line, indented 4 spaces from the "obj". … |
chipx86
|
|
|
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
This shouldn't be here. |
chipx86
|
|
|
"review request's" Also, trailing space should be removed. |
chipx86
|
|
|
Col: 40 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 40 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 40 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
Col: 41 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 41 E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
Col: 41 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 41 E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
Col: 41 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 41 E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
Col: 39 E711 comparison to None should be 'if cond is None |
reviewbot
|
|
|
Col: 41 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
I think this is worth some documentation. |
 mike_conley
|
|
|
Col: 41 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
This isn't something the others are doing yet, but you should use newer-style parent construction: super(DependsOnColumn, self).__init__(self, *args, **kwargs) |
chipx86
|
|
|
No blank line here. |
chipx86
|
|
|
I'm seeing some stuff like this that I've commented on on previous reviews, which were marked as fixed. Can you … |
chipx86
|
|
|
If you're going to break this up like this, I think I'd prefer to be consistent about putting the space … |
mike_conley
|
|
|
Don't change things that aren't directly related to your change. |
chipx86
|
|
|
Same. |
chipx86
|
|
|
Even if pep8 complains, modifying this in your change only makes it harder for us to track down issues later. … |
chipx86
|
|
|
Nope, put it back :) |
chipx86
|
|
|
"by" should be wrapped with trans tags (although the ordering when translated might differ...by might come after...)... I assume you … |
mike_conley
|
|
|
ReviewBot might have a point. Maybe: obj = ReviewRequest.objects.get( Q(local_id=value) & Q(local_site=local_site)) |
mike_conley
|
|
|
Col: 41 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Undo this change. |
mike_conley
|
|
|
Undo this change. |
mike_conley
|
|
|
Undo this change. |
mike_conley
|
|
|
Undo this change. |
mike_conley
|
|
|
Undo this change. |
mike_conley
|
|
|
Undo this change. |
mike_conley
|
|
|
Undo these changes. |
mike_conley
|
|
|
Undo this change. |
mike_conley
|
|
|
Undo this change. |
mike_conley
|
|
|
Undo this change. |
mike_conley
|
|
|
Undo this change. |
mike_conley
|
|
|
Undo this change. |
mike_conley
|
|
|
Undo this change. |
mike_conley
|
|
|
Undo these changes. |
mike_conley
|
|
|
Undo thsi change. |
mike_conley
|
|
|
Undo this change. |
mike_conley
|
|
|
Undo these changes. |
mike_conley
|
|
|
Undo this change. |
mike_conley
|
|
|
Undo this change. |
mike_conley
|
|
|
Col: 41 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Undo this change. |
mike_conley
|
|
|
related_name should never be localized. No _(..) |
chipx86
|
|
|
Alignment issues. The parameters should be aligned to 'ReviewRequest'. The 'ReviewRequest' here should be able to be ReviewRequest (no quotes). |
chipx86
|
|
|
Col: 40 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 40 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 40 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Same here. |
chipx86
|
|
|
These should really be ===. Can you change all three? |
chipx86
|
|
|
This looks broken. I don't see how it can compile. |
chipx86
|
|
|
Where did this go? |
chipx86
|
|
|
=== |
chipx86
|
|
|
"Review request" |
chipx86
|
|
|
=== |
chipx86
|
|
|
"Review requests" |
chipx86
|
|
|
These should be unindented, but with the contents indented relative to the nearest template tags (like: {% foo %}). Same … |
chipx86
|
|
|
class="" should be removed. |
chipx86
|
|
|
"for this review request." |
chipx86
|
|
|
"for this review request." |
chipx86
|
|
|
"for this review request." |
chipx86
|
|
|
We have this split in two places, and both are slightly different. One is "[, ]+" and one is "[,\s*]". … |
chipx86
|
|
|
Rather than this, can you add a new function to ReviewRequestManager (reviews/managers.py) called, let's say, for_id(), that would contain this … |
chipx86
|
|
|
pk=value |
chipx86
|
|
|
Col: 41 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
No blank line here. |
chipx86
|
|
|
"review request's" |
chipx86
|
|
|
Col: 80 E501 line too long (85 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (85 > 79 characters) |
reviewbot
|
|
|
I believe verbose_name needs to be wrapped in _() |
mike_conley
|
|
|
Same here |
mike_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) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 45 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 32 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
What happened to Christian's request? "Yeah, we should use the one we have in reviews/models.py." See the comments in my … |
mike_conley
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
DE
- Description:
-
- Removed original architecture for dependencies, and replaces
it with a slimmed down m2m for now. So far allows ReviewRequests
to depend on other ReviewRequests and the opposite for blocking,
as well as review requests drafts to depend on review requests.
- Admin panel editing only atm.
- Removing remenants of old code from previous design.
~ - Also, did a run through of PEP8 standards on a portion of
touched files for a mid-task coding standards cleanup.
~ - Also, did a run through of PEP8 standards on a portion of
touched files (reviews/models.py, ./admin.py)for a mid-task
coding standards cleanup.
- Removed original architecture for dependencies, and replaces
DE
- People:
-
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.
DE
- Change Summary:
-
Edit for better description and deleted the other request as it was merely for testing purposes.
- Description:
-
~ - Removed original architecture for dependencies, and replaces
it with a slimmed down m2m for now. So far allows ReviewRequests
to depend on other ReviewRequests and the opposite for blocking,
as well as review requests drafts to depend on review requests.
~ - Admin panel editing only atm.
~ - Removing remenants of old code from previous design.
~ - Also, did a run through of PEP8 standards on a portion of
touched files (reviews/models.py, ./admin.py)for a mid-task
coding standards cleanup.
~ 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 simply adds this feature's back-end and admin panel + access. Therefore, you are able to view these relationships by + adding new relationships via the review request/draft review request + "depends on" field area in the admin database section. + - The next code iteration has the goal of adding front-end editing + support. + + Change Log
+ - Removed original architecture for dependencies, and replaces + it with a slimmed down m2m for now. So far allows ReviewRequests + to depend on other ReviewRequests and the opposite for blocking, + as well as review requests drafts to depend on review requests. + - Admin panel editing only atm. + - Removing remenants of old code from previous design. + - Also, did a run through of PEP8 standards on a portion of + touched files (reviews/models.py, ./admin.py)for a mid-task + coding standards cleanup. - Removed original architecture for dependencies, and replaces
GR
-
-
Since djblets is part of Django and reviewboard is part of Reviewboard, I think there should exists a blank line between their imports.
-
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.
-
DE
- Change Summary:
-
First non-WIP Submission.
- Summary:
-
*[WIP]* - Redone Dependencies: Request <-> Request AND Draft Request -> Request FROM Admin PanelDepends On & Blocks Relationships to Request Box.
- Description:
-
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 simply adds this feature's back-end and admin panel ~ access. Therefore, you are able to view these relationships by ~ adding new relationships via the review request/draft review request ~ "depends on" field area in the admin database section. ~ - 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 build as a reverse of dependency, and ~ thus are automatically generated upon dependency change. - - The next code iteration has the goal of adding front-end editing - support. - - Change Log
- - Removed original architecture for dependencies, and replaces - it with a slimmed down m2m for now. So far allows ReviewRequests - to depend on other ReviewRequests and the opposite for blocking, - as well as review requests drafts to depend on review requests. - - Admin panel editing only atm. - - Removing remenants of old code from previous design. - - Also, did a run through of PEP8 standards on a portion of - touched files (reviews/models.py, ./admin.py)for a mid-task - coding standards cleanup. - Testing Done:
-
+ 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. - Diff:
-
Revision 2 (+197 -95)
-
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 -
-
-
-
-
-
-
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 -
-
-
-
-
-
-
DE
- Description:
-
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 build as a reverse of dependency, and ~ thus are automatically generated upon dependency change. ~ - Blocking relationships are built as a reverse of dependency, and ~ thus are automatically generated upon a dependency change.
-
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 -
-
-
-
-
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 -
-
-
-
-
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.)
-
-
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.
-
-
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.
-
_() is for localizing human-readable strings. This is an identifier, which won't work so well if, say, a Russian translation changes it.
-
-
-
Can you change these to use === instead of == ? (In JavaScript, == casts, and === doesn't.. the latter is faster).
-
-
-
Note the other template tags. The {% should always be unindented, and the contents inside should be indented relative to the other template tags. -
-
-
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?
-
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.
-
-
DE
- Change Summary:
-
Addressed auto-PEP8 incident by removing unneeded changes.
- Diff:
-
Revision 6 (+387 -108)
-
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 -
-
-
-
DE
- Change Summary:
-
Addressed Bot and issue tracker.
- Diff:
-
Revision 7 (+386 -107)
-
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 -
-
-
-
-
DE
- Diff:
-
Revision 8 (+389 -107)
-
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 -
-
DE
- Diff:
-
Revision 9 (+389 -107)
-
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 -
-
DE
- Diff:
-
Revision 10 (+391 -107)
-
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 -
-
DE
- Diff:
-
Revision 11 (+391 -107)
-
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 -
-
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 -
-
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
-
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?
-
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).
-
"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?
-
ReviewBot might have a point. Maybe: obj = ReviewRequest.objects.get( Q(local_id=value) & Q(local_site=local_site))
-
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.
-
This isn't something the others are doing yet, but you should use newer-style parent construction: super(DependsOnColumn, self).__init__(self, *args, **kwargs) -
-
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?
-
-
-
Even if pep8 complains, modifying this in your change only makes it harder for us to track down issues later. It's unrelated.
-
DE
- Change Summary:
-
Hopefully this new diff will have addressed the accidental PeP8 un-fixing.
- Diff:
-
Revision 13 (+187 -92)
-
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 -
-
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
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
DE
- Change Summary:
-
I could type a story about this whole ordeal, but in short, I found an externally saved copy of a version of the development that existed at a safe point before the whole branch collapse occurred, so I'm overhauled it to where it needed to be, and it should be outside of these accidentally included changes from the bad merge incident.
-
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 -
-
-
-
-
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 -
-
-
-
Alignment issues. The parameters should be aligned to 'ReviewRequest'. The 'ReviewRequest' here should be able to be ReviewRequest (no quotes).
-
-
-
-
-
-
-
-
-
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. -
-
-
-
-
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.
-
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).
-
-
-
-
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 -
-
-
-
-
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 -
-
GR
-
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...
-
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 %} -
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/>
-
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 -
-
-
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 -
-
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 -
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