Status API part 3: Refactor entries on review detail page.
Review Request #8339 — Created Aug. 19, 2016 and submitted
The review detail page is huge and ugly and multi-tentacled. This change
attempts to clean things up just a little bit, by creating some classes for the
various types of "entries" (the review and change boxes).This is primarily code motion, but I did discover an obscure bug while I was
doing this related to default expansion for draft reply comments (theentry
variable was never assigned in that case, leaving it to be whatever it was
before).There's obviously a ton more that could be done here, but I want to take this
one step at a time.
Verified the appearance of the review detail page with multiple reviews,
replies, and change descriptions. Everything works as it did before.
| Description | From | Last Updated |
|---|---|---|
|
Does .. seealso: :py:data:`...` work here? |
 brennie
|
|
|
Can we call this something like BaseReviewRequestPageEntry? It'll be more clear what it's for when reading other code. |
 chipx86
|
|
|
Can you elaborate on this, going into what an "entry" means? There's a lot going on on this page, and … |
chipx86
|
|
|
"JavaScript" |
chipx86
|
|
|
bool |
chipx86
|
|
|
bool |
chipx86
|
|
|
:py:attr: Referenced attributes (or any we want to make public, really) also need to be listed in the class's docstring … |
chipx86
|
|
|
This uses keyword arguments, but the others don't. We should be consistent here in our usage. (I'd say probably go … |
chipx86
|
|
|
Missing period. |
chipx86
|
|
|
Can we use keyword arguments, so this is self-documenting? |
chipx86
|
|
|
Might be nicer to just pull this out into a fieldsets variable first, and then iterate through that. |
chipx86
|
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+311 -233) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html
-
Great cleanup :) I like where this could go.
Got some comments about naming and docs, but the logic seems fine. Tried going through the collapsing logic and I think it looks good.
Could the replies end up being moved into the entries, and the "should this be collapsed" logic? I don't care about that for this change, just wondering about planned next steps.
-
reviewboard/reviews/detail.py (Diff revision 2) Can we call this something like
BaseReviewRequestPageEntry? It'll be more clear what it's for when reading other code. -
reviewboard/reviews/detail.py (Diff revision 2) Can you elaborate on this, going into what an "entry" means? There's a lot going on on this page, and I want new contributors to be clear on this.
-
-
reviewboard/reviews/detail.py (Diff revision 2) :py:attr:Referenced attributes (or any we want to make public, really) also need to be listed in the class's docstring under "Attributes:"
-
reviewboard/reviews/detail.py (Diff revision 2) This uses keyword arguments, but the others don't. We should be consistent here in our usage. (I'd say probably go with keyword arguments everywhere.)
-
-
reviewboard/reviews/detail.py (Diff revision 2) Can we use keyword arguments, so this is self-documenting?
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+344 -233) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html
-
-
-
-
reviewboard/reviews/detail.py (Diff revisions 2 - 3) Might be nicer to just pull this out into a
fieldsetsvariable first, and then iterate through that.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+346 -233) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html