Add complexity icons to the commits in the diffviewer.
Review Request #6943 — Created Feb. 12, 2015 and submitted
The
DiffCommitIndexView
now renders complexity icons via the
DiffComplexityIconView
that theDiffIndexView
also uses.
Diff complexity icons appear next to commits in the
diffviewer's commit list.
Description | From | Last Updated |
---|---|---|
Does prefetch_related do what you want? |
david | |
Comments here are a little weirdly aligned. |
david | |
Because this is javascript land, we actually need to do this wacky thing: for (lineType in this.attributes.lineCounts) { if (this.attributes.lineCounts.hasOwnProperty(lineType)) … |
david | |
We already have much of this and the one below. Let's pull those out into something reusable. The review request … |
chipx86 | |
Can we put the comments on the lines before the attributes? Helps to allow more verbose comments and to prevent … |
chipx86 | |
Just a thing to consider: I'm not 100% sure, but I suspect that an object doing diffcommit.get('lineCounts').deletes = 5 could … |
chipx86 | |
I we pull out lineCounts into a local variable, we'll reduce the number of property accesses required. |
chipx86 | |
Indentation is off here. |
david | |
I think this should be defined inside the this.collection.each() callback function. |
david |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py Ignored Files: reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js reviewboard/static/rb/css/pages/reviews.less Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py Ignored Files: reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js reviewboard/static/rb/css/pages/reviews.less
-
-
-
reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js (Diff revision 2) Comments here are a little weirdly aligned.
-
reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js (Diff revision 2) Because this is javascript land, we actually need to do this wacky thing:
for (lineType in this.attributes.lineCounts) { if (this.attributes.lineCounts.hasOwnProperty(lineType)) { sum += this.attributes.lineCounts[lineType]; } }
Alternatively,
_.each
will do the right thing when iterating over object properties.
Change Summary:
Address David's issues.
Testing Done: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+69 -6) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py Ignored Files: reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js reviewboard/static/rb/css/pages/reviews.less Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py Ignored Files: reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js reviewboard/static/rb/css/pages/reviews.less
-
-
reviewboard/static/rb/css/pages/reviews.less (Diff revision 3) We already have much of this and the one below. Let's pull those out into something reusable.
The review request page should be pulling in
diffviewer.less
, so you can just have those all in one place there. -
reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js (Diff revision 3) Can we put the comments on the lines before the attributes? Helps to allow more verbose comments and to prevent annoyances when we add new attributes (which may have longer names or defaults).
-
reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js (Diff revision 3) Just a thing to consider:
I'm not 100% sure, but I suspect that an object doing
diffcommit.get('lineCounts').deletes = 5
could end up overwriting the defaults for other instances. I don't know if Backbone makes a copy of dictionary values when populating from defaults. Just something to think about for any future changes. -
reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 3) I we pull out lineCounts into a local variable, we'll reduce the number of property accesses required.
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py Ignored Files: reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py Ignored Files: reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
-
-
reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js (Diff revision 4) Indentation is off here.
-
reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 4) I think this should be defined inside the
this.collection.each()
callback function.
-
Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py Ignored Files: reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py Ignored Files: reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js