Revision selectors

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

Information

Review Board

Reviewers

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. Show all issues

    __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. Show all issues

    This won't quite work. Instead, you should use this:

    {
        attachmentRevisionText: iterpolate(
            gettext('Attachment Revision %s'),
            [this.options.revision])
    }
    
  4. Show all issues

    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. Show all issues

    This comment is wrong.

  6. Show all issues

    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.

  7. Show all issues

    What is '4'?

  8. Show all issues

    This should use _super and call

  9. Show all issues

    This comment is wrong.

  10. Show all issues

    This should use _super and call

  11. Show all issues

    This fits on one line

  12. Show all issues

    I don't know what this comment is about.

  13. Show all issues

    Remove this line?

  14. Show all issues

    Add a blank line between these.

  15. Show all issues

    _super

  16. Show all issues

    _super and call

  17. Show all issues

    _super and call

  18. Show all issues

    _super and call

  19. reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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.

  20. 
      
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. Show all issues

    Indentation is super weird here.

  3. Show all issues

    This comment can go away.

  4. Show all issues

    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 :(

  5. Show all issues

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

  6. Show all issues

    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.

  7. Show all issues

    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.

  8. Show all issues

    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?

  9. Show all issues

    This should use _super and call

  10. Show all issues

    Shouldn't have an array around ev.

  11. 
      
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/