Revision selectors

Review Request #6120 — Created July 19, 2014 and discarded

volodymyr
Review Board
5824
reviewboard, students

Adding revision selector for attachments. To avoid code duplication with existing selector for diffs, a common parent class called CommonRevisionSelectorView was created.

Tested as part of https://reviews.reviewboard.org/r/5824/

Description From Last Updated

__super__ isn't a public part of backbone's API. Instead, we should be using our own _super method: _super(this).initialize.apply(this, arguments);

daviddavid

This won't quite work. Instead, you should use this: { attachmentRevisionText: iterpolate( gettext('Attachment Revision %s'), [this.options.revision]) }

daviddavid

Same comment about __super__. Because you're specifying your own arguments, call is a better choice than apply: return _super(this).render.call( this, ...

daviddavid

This comment is wrong.

daviddavid

There's no value variable delclared in this method, which means that this is global. It also seems like this method ...

daviddavid

What is '4'?

daviddavid

This should use _super and call

daviddavid

This comment is wrong.

daviddavid

This should use _super and call

daviddavid

This fits on one line

daviddavid

I don't know what this comment is about.

daviddavid

Remove this line?

daviddavid

Add a blank line between these.

daviddavid

_super

daviddavid

_super and call

daviddavid

_super and call

daviddavid

_super and call

daviddavid

This does exactly what the parent class' _onLabelClick implementation does, so you can remove it.

daviddavid

Indentation is super weird here.

daviddavid

This comment can go away.

daviddavid

Did you mean to be passing an array for the argument? I suspect not. Also, this would all fit on ...

daviddavid

Did you mean to be passing an array for the argument? I suspect not.

daviddavid

We should have slightly different labels between these two. For diffs, we want ['orig', '1', '2', '3', ...], because the ...

daviddavid

Doesn't this end up adding one too many ticks for file attachments?

daviddavid

You're passing one single argument to the super render, which is an array of stuff. Based on your implementation above, ...

daviddavid

This should use _super and call

daviddavid

Shouldn't have an array around ev.

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
        reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
        reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
        reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
        reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
    
    
  2. 
      
david
  1. 
      
  2. __super__ isn't a public part of backbone's API. Instead, we should be using our own _super method:

    _super(this).initialize.apply(this, arguments);
    
  3. This won't quite work. Instead, you should use this:

    {
        attachmentRevisionText: iterpolate(
            gettext('Attachment Revision %s'),
            [this.options.revision])
    }
    
  4. Same comment about __super__.

    Because you're specifying your own arguments, call is a better choice than apply:

    return _super(this).render.call(
        this, this.options.numRevisions, true, 1);
    
  5. There's no value variable delclared in this method, which means that this is global.

    It also seems like this method half assumes that there can be multiple handles, and half assumes that there's a single value.

  6. I don't know what this comment is about.

  7. Add a blank line between these.

  8. reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This does exactly what the parent class' _onLabelClick implementation does, so you can remove it.

    1. Sorry, I hadn't noticed the slight difference between the two implementations.

  9. 
      
VO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
        reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
        reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
        reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
        reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
    
    
  2. 
      
david
  1. 
      
  2. Indentation is super weird here.

  3. Did you mean to be passing an array for the argument? I suspect not. Also, this would all fit on one line.

    1. All of these were arrays when apply function was used. I forgot to fix them :(

  4. Did you mean to be passing an array for the argument? I suspect not.

  5. We should have slightly different labels between these two.

    For diffs, we want ['orig', '1', '2', '3', ...], because the options are to diff between the original file and one of the revisions, or between two of the revisions.

    For file attachments, there's no 'orig', since there's no upstream version of the file. We therefore should have ['1', '2', '3', ...]

    1. I started numeration from 0 if that's OK. If not, I'll fix it.

  6. Doesn't this end up adding one too many ticks for file attachments?

    1. Right now numRevisions is numTotal - 1, mimicing revisions definition for diffs. We once discussed this in https://reviews.reviewboard.org/r/5824/ (that issue is still open). I think it might be a bit misleading to have different definitions for revisions in different cases (maybe it should be called otherwise then), e.g. numTotal for attachments and numTotal - 1 for diffs. While taking the defition from diffs isn't perfect as well, I think it works better, because:
      1) We actually are uprgarding a file attachement - there is an initial version and subsequent revisions to it.
      2) In several places of the code we won't have to do

      if (isFileAttachment) {
      numRevisions = ...;
      } else {
      numRevisions = ...;
      }

      and just use the same logic in both cases.

  7. You're passing one single argument to the super render, which is an array of stuff. Based on your implementation above, each of these should just be additional arguments to call.

    Can you thoroughly test diffs and interdiffs to make sure you're not causing any regressions?

  8. This should use _super and call

  9. Shouldn't have an array around ev.

  10. 
      
VO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
        reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
        reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
        reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
        reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
    
    
  2. 
      
VO
Review request changed

Status: Discarded

Change Summary:

Obsoleted by /r/6342/
Loading...