Add complexity icons to the commits in the diffviewer.

Review Request #6943 — Created Feb. 12, 2015 and submitted

Information

Review Board
dvcs

Reviewers

The DiffCommitIndexView now renders complexity icons via the
DiffComplexityIconView that the DiffIndexView 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?

daviddavid

Comments here are a little weirdly aligned.

daviddavid

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)) …

daviddavid

We already have much of this and the one below. Let's pull those out into something reusable. The review request …

chipx86chipx86

Can we put the comments on the lines before the attributes? Helps to allow more verbose comments and to prevent …

chipx86chipx86

Just a thing to consider: I'm not 100% sure, but I suspect that an object doing diffcommit.get('lineCounts').deletes = 5 could …

chipx86chipx86

I we pull out lineCounts into a local variable, we'll reduce the number of property accesses required.

chipx86chipx86

Indentation is off here.

daviddavid

I think this should be defined inside the this.collection.each() callback function.

daviddavid
reviewbot
  1. 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
    
    
  2. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/diffviewer/views.py (Diff revision 2)
     
     

    Does prefetch_related do what you want?

    1. Not really. As the diffset object has already been fetched, it doesn't have the ability (as far as I can see) to fetch the related objects. I believe it would have tbe done wherever the diffset is fetched.

    2. Actually it turns out this is exactly what I want. I was just attempting to use it in the wrong way.

  3. Comments here are a little weirdly aligned.

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

  5. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. 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.

    1. These aren't actually used in the review request page, so I removed them and references to them. Putting complexity icons on the review request page will be another change.

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

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

    1. As it turns out, it will not copy and can overwrite defaults for other instances.

  5. I we pull out lineCounts into a local variable, we'll reduce the number of property accesses required.

  6. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. Indentation is off here.

  3. I think this should be defined inside the this.collection.each() callback function.

  4. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to dvcs (22e2cef)
Loading...