Add Bookmarking feature to Diff Viewer.

Review Request #7658 — Created Sept. 25, 2015 and updated

Information

Review Board
master

Reviewers

Add bookmarking feature in diff viewer so users can create and quickly jump
between bookmarks. Intended to help users moving back and forth between
different parts in the diff viewer when doing code reviews.

A "ghost" bookmark will follow the cursor on the right margin of the diff
viewer, which a user can click on to create new bookmarks on the hovered line
of the diff file. Clicking on the created bookmark will scroll the window
to where the bookmarked line is. Bookmarks are saved per diff using browser
Cookie, and will persist for one week.

Usability features include assigning different colors to bookmarks for easy
differentiation, positioning bookmarks on where the target line is relative
to the whole page, and animating the position at bookmark creation so users
can see where the newly created bookmark will reside.

Automated JS test cases:

Ghost bookmark
- should move along with the cursor on the diff
- becomes invisible when the cursor is not on diff
- expands when the cursor approaches
- shrinks when the cursor moves away
- creates a new bookmark when clicked on it
- no ghost bookmark when there already is a bookmark on the hovered line

Bookmarks
- appears in front of ghost bookmark
- expands when the cursor approaches
- shrinks when the cursor moves away
- will animate on creation
- on file header should only display the filename
- on a line in a file should display the filename and the line number
- on an expanded chunk will hide when the chunk is collapsed
- on a collapsed chunk will appear when the chunk is expanded
- should have different colors (for first 10 bookmarks)
- will be removed when clicked on [x]
- will scroll the page when clicked
- can be created from cookies
- will trigger cookie save event on creation or removal

Following features are tested manually on local devserver:

Bookmarks
- on an empty line should display nicely
- on a line that wraps on browser as multiple lines should display nicely
- added bookmark will appear again after refreshing on the page
- removed bookmarks will not appear again after refreshing on the page
- should be positioned where the main bookmark is relative to the whole page
- are repositioned when window is resized
- are repositioned when a chunk is expanded/collapsed
- are repositioned when a file is loaded


Description From Last Updated

Space after selector.

brenniebrennie

Alphabetize these.

brenniebrennie

We should set a constant in defs.less for this. e.g. @ghost-bookmark-background-color: rgba(173, 173, 173, 0.5);

brenniebrennie

Can we call this RB.MookmarkModel?

brenniebrennie

There should be a defaults object that contains safe default values (although fileName would probably be null in that case)

brenniebrennie

diffID

brenniebrennie

interdiffID

brenniebrennie

We really don't want to have models stored in models. Backbone doesn't support it that well. Instead, you can pass …

brenniebrennie

This should be in collections/bookmarkCollection.js

brenniebrennie

Likewise, RB.BookmarkCollection ?

brenniebrennie

Missing semicolon.

brenniebrennie

RB.BookmarkContainerView Also, this should be in its own separate file.

brenniebrennie

We use lowerCamelCase for JS names and hyphen-case for HTML names, so this should be bookmark-container.

brenniebrennie

Is this supposed to return something? If not, we do not need the return statement.

brenniebrennie

Please format this like: if (!noRender) { bookmarkerView.render(); }

brenniebrennie

We use one var statement as the first statement in any function.

brenniebrennie

You are likely going to want to use _.throttle here. Listening for mousemoves is expensive.

brenniebrennie

We put a space between the ) and the {. Please fix this throughout.

daviddavid

Should be bookmark (not Bookmark)

daviddavid

This line isn't necessary.

daviddavid

Variables should start with a lower-case letter.

daviddavid

You don't need the !! here since it's just a truthy-test. All conditionals should also have {}, regardless of whether …

daviddavid

It's probably better to format this with one call per line: this.$ghostBookmark .removeClass('open') .find('.bookmark-fold') .css('width', 0);

daviddavid

This stuff should probably happen in render rather than initialize.

daviddavid

Make sure you're indenting 4 spaces in JavaScript code.

daviddavid

Make sure you're indenting 4 spaces in JavaScript code.

daviddavid

The ?: operator will already convert to a bool, so you don't need !!

daviddavid

You can use _.bind here, too.

daviddavid

This should be the last line in the file. This is a vim modeline which gives the editor settings for …

brenniebrennie

You can nest this in the .close rule with &:hover.

brenniebrennie

You should use .transition(). It does a prefixed version of transition.

brenniebrennie

"the diffviewer" Needs a period.

brenniebrennie

I'm not sure if the cookie data should be passed in via options or not. You should maybe ping Christian …

brenniebrennie

Put these on seperate lines.

brenniebrennie

Remove this comment.

brenniebrennie

We use jquery.cookie. You can use it for reading/writing cookies. Check out the docs

brenniebrennie

Can we call this cookie ?

brenniebrennie

Blank line between these.

brenniebrennie

Remove this comment.

brenniebrennie

Replace this with a for loop.

brenniebrennie

Since we're not actually transmuting the models or options, I feel like we should do this in a signal handler. …

brenniebrennie

Needs a doc comment.

brenniebrennie

Put these on seperate lines, with defined variables first.

brenniebrennie

Can we call this bookmark ?

brenniebrennie

remove this comment.

brenniebrennie

This overwrites all of the cookie that is set, doesn't it? We really should use $.cookie here.

brenniebrennie

Needs a period.

brenniebrennie

Needs a doc comment.

brenniebrennie

Just call it template. That is the convention we use.

brenniebrennie

Space after , between items.

brenniebrennie

Remove this comment.

brenniebrennie

This should use sentence casing, e.g., begin with a capital and end with a period. "from a clone of the"

brenniebrennie

Missing a var statement. e isn't a descriptive variable name; we should use bookmark instead.

brenniebrennie

Since you're doing a filter & map on the values and only using them once, we can just do a …

brenniebrennie

Backbone.Model.get only takes one key at a time, so this should be: e.get('fileDiffID'), e.get('interFileDiffID')

brenniebrennie

If this is private it should be prefixed with an underscore. Also, this needs a doccomment.

brenniebrennie

Remove this line.

brenniebrennie

view is more readable than bview.

brenniebrennie

We format multi-line comments in JS as follows: /* * Line 1. * Line 2. * ... */

brenniebrennie

You can do view.remove().render() because remove should return the view.

brenniebrennie

Same comment as above regards to public/private-ness and doc comment.

brenniebrennie

Should be prefixed with underscore. Needs doc comment.

brenniebrennie

Should be prefixed with underscore. Needs doc comment.

brenniebrennie

Blank line between these.

brenniebrennie

Doc comment.

brenniebrennie

var $target = $(e.target), fileName, line, top;

brenniebrennie

Blank line between statement and block.

brenniebrennie

See above re: multi-line comments.

brenniebrennie

This is quite hard to read. I'm not sure what nodeType == 3 means? Is there a constnat we can …

brenniebrennie

Should be indented 4 spaces not 2.

brenniebrennie

What do you mean, without updating obj?

brenniebrennie

Sentence casing.

brenniebrennie

} else {

brenniebrennie

doc comment.

brenniebrennie

Doc comment.

brenniebrennie

We prefix jQuery objects, so this should be $diffViewElem. Also, you can shorten Elem to El.

brenniebrennie

Needs doc comment.

brenniebrennie

Should be indented 4.

brenniebrennie

var fileDiff = file.get('filediff'), interFileDiff = file.get('interfilediff');

brenniebrennie

We can write this more clearly as : return (filediff.id === fileDiffID && ((interFileDiff === null && interFileDiffID === null) …

brenniebrennie

===

brenniebrennie

Indent multiples of 4 spaces.

brenniebrennie

Missing a period.

brenniebrennie

Missing a period.

brenniebrennie

Col: 27 W601 .has_key() is deprecated, use 'in'

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Can we rename the cookie to "diff-bookmarks" ? Or maybe we should just have it be "bookmarks" (as we may …

chipx86chipx86

Just some tweaks to this. The argument name should include the type, and for request (a very common argument), we …

chipx86chipx86

This will be faster as: bookmark_cookie = request.COOKIES.get('diffbookmarks', '')

chipx86chipx86

We namespace these names, so these would be best as @bookmark-ghost-bg-color and @bookmark-default-bg-color, in alphabetical order.

chipx86chipx86

"Bookmarks"

chipx86chipx86

I know we don't have this in many places, but I'm trying to get in the habit: Can you add …

chipx86chipx86

We always use the long form: #FFFF00. Should also have a constant.

chipx86chipx86

All z-indexes must be registered in defs.less as constants.

chipx86chipx86

Not sure what e() is, but elsewhere, we do: .transition(padding-left 0.2s linear, top 0.2s ease-out;); (Note the inner ; near …

chipx86chipx86

We use the form of: 1px #333 solid This also needs a constant defined. Same with all other colors.

chipx86chipx86

All repeating sizes for padding, fonts, etc. should have a constant.

chipx86chipx86

Here and elsewhere, make sure there's a space before {.

chipx86chipx86

Selector groups are always in this order: & { ... } &#id { ... } &:pseudo-class { ... } &.class-name …

chipx86chipx86

No capitalization of "bookmarks" needed. Can you have this go into detail on what the class does? What it's tracking, …

chipx86chipx86

Multi-line comments must always be in this form: /* * line * line */ Same with other comments below.

chipx86chipx86

Function/method docs must be in this form: /* * One-line summary. * * Multi-line description. */ When there are arguments …

chipx86chipx86

Blank line between these.

chipx86chipx86

Blank line between these.

chipx86chipx86

Space before {.

chipx86chipx86

Let's pull out model.get('fileDiffID') just once.

chipx86chipx86

This would be better named as "updatedCookie". Make sure this is documented in the method docs.

chipx86chipx86

No blank line here.

chipx86chipx86

We only use one var statement per function, with one variable per line, like: var collection, listenerObj;

chipx86chipx86

I don't know that this is really needed. If you just need to check that the event emitted properly, you …

chipx86chipx86

Space before {. No need for return;, since that's implied.

chipx86chipx86

Ideally, you should use .toBe instead of .toEqual in tests. The reason is that .toBe is equivalent to strict JavaScript …

chipx86chipx86

Can you add docs for each of these?

chipx86chipx86

Just like methods, this should be in the form of: /* * One-line summary. * * Multi-line description. */ This …

chipx86chipx86

This is the default, so you don't need this here.

chipx86chipx86

When named template, it implies it's the template for the view itself. You probably want to call this ghostBookmarkTemplate.

chipx86chipx86

The strings must align. Indentation should be within the string, 1 space.

chipx86chipx86

Each method needs a docstring.

chipx86chipx86

Element-related initialization should go in render.

chipx86chipx86

Any member variables that don't need public access should be made private (prefix the name with a _).

chipx86chipx86

render must return this at the end.

chipx86chipx86

render shouldn't be responsible for deciding where its main element is put. That's the responsibility of the caller, only.

chipx86chipx86

These should instead go in events, like: events: { 'click .ghost': '_onGhostBookmarkClicked', ... }

chipx86chipx86

That sounds odd to me, but you should definitely be doing: return _super(this).remove.call(this); instead of removing it manually. The parent …

chipx86chipx86

Can you tell me what the body query is about? Is this about whether it's currently packed into the page …

chipx86chipx86

What's this for? Ideally, you wouldn't need to do this. I want to be sure I understand it well.

chipx86chipx86

The list of variables should begin with defined variables, so: var cookie = JSON.parse(cookieString), bookmark;

chipx86chipx86

i must be declared above, in the existing var list.

chipx86chipx86

All jQuery-wrapped elememnts should be named like $diffReviewable.

chipx86chipx86

This is a bit confusing. Can you instead just do this work when binding the element? If you're just looking …

chipx86chipx86

One var statement.

chipx86chipx86

.parents() is very expensive. Is there any way we can be smarter about what we're looing up? Can you explain …

chipx86chipx86

Blank line after this.

chipx86chipx86

This can be one statement, and you can probably use .width(). How about: var $bookmarkText = this._$ghostBookmark.find('.bookmark-text'); this._$ghostBookmark.find('.bookmark-fold') .width($bookmarkText.outerWidth()); Also, …

chipx86chipx86

Sentence capitalization and period.

chipx86chipx86

The .css should be indented one.

chipx86chipx86

These should go first, since they're defined.

chipx86chipx86

.parents() is very expensive, so we shoud minimize this however we can. Certainly, we should never call it more than …

chipx86chipx86

This is very awkwardly formatted. can we pull some of this out somehow?

chipx86chipx86

This has already been pulled out of diffModel.

chipx86chipx86

Only one blank line here.

chipx86chipx86

Space before {.

chipx86chipx86

filename +=.

chipx86chipx86

You can chain these.

chipx86chipx86

Blank line between these.

chipx86chipx86

Values should align.

chipx86chipx86

Blank line between these.

chipx86chipx86

Needs docs.

chipx86chipx86

Same comments as above with the formatting.

chipx86chipx86

This will cause problems. You need to instead do: this.$el.html(this.template()); Also, this must happen in render().

chipx86chipx86

Anything that isn't public API should have a _ in front of it.

chipx86chipx86

Blank line before comments.

chipx86chipx86

Would be best to pull these out into variables, since other things seem to use these.

chipx86chipx86

Blank lines before comments.

chipx86chipx86

If it's just one property, do: this.$el.css('top', topPosition);

chipx86chipx86

Blank line between these. vars and returns generally should be thought of as their own "paragraphs" of code.

chipx86chipx86

Parameters must align.

chipx86chipx86

Blank line between these.

chipx86chipx86

Need a space before {.

chipx86chipx86

Because of how JavaScript works, this can pick up data you don't expect. You need to use this form: for …

chipx86chipx86

Best to only pull this out once.

chipx86chipx86

Indentation is wrong. !== should be used instead of !=.

chipx86chipx86

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Should ideally be in alphabetical order.

chipx86chipx86

Here too.

chipx86chipx86

And these.

chipx86chipx86

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
  2. 
      
brennie
  1. 
      
  2. Show all issues

    Space after selector.

  3. reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Alphabetize these.

  4. Show all issues

    We should set a constant in defs.less for this.

    e.g.

    @ghost-bookmark-background-color: rgba(173, 173, 173, 0.5);
    
  5. Show all issues

    Can we call this RB.MookmarkModel?

  6. reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues

    There should be a defaults object that contains safe default values (although fileName would probably be null in that case)

  7. Show all issues

    diffID

  8. Show all issues

    interdiffID

  9. Show all issues

    We really don't want to have models stored in models. Backbone doesn't support it that well. Instead, you can pass diffModel in options, e.g.

    var bookmarkModel = new RB.BookmarkerModel({
        diffModel: diffModel
    });
    

    and then access it either through this.options or options (which is passed as the first argument to initialize).

  10. Show all issues

    This should be in collections/bookmarkCollection.js

  11. Show all issues

    Likewise, RB.BookmarkCollection ?

  12. Show all issues

    Missing semicolon.

  13. Show all issues

    RB.BookmarkContainerView

    Also, this should be in its own separate file.

  14. Show all issues

    We use lowerCamelCase for JS names and hyphen-case for HTML names, so this should be bookmark-container.

  15. Show all issues

    Is this supposed to return something? If not, we do not need the return statement.

  16. Show all issues

    Please format this like:

    if (!noRender) {
        bookmarkerView.render();
    }
    
  17. Show all issues

    We use one var statement as the first statement in any function.

  18. Show all issues

    You are likely going to want to use _.throttle here. Listening for mousemoves is expensive.

  19. 
      
chipx86
  1. Can you upload screenshots showing how this works?

  2. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
YN
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
david
  1. So I don't have a lot of comments on the code yet, since it looks like you're still in early exploration.

    However, I do want to help guide you into our coding style now, before you write too much code. These comments are primarily about that.

  2. Show all issues

    We put a space between the ) and the {. Please fix this throughout.

  3. Show all issues

    Should be bookmark (not Bookmark)

  4. Show all issues

    This line isn't necessary.

  5. Show all issues

    Variables should start with a lower-case letter.

  6. Show all issues

    You don't need the !! here since it's just a truthy-test.

    All conditionals should also have {}, regardless of whether the block therein is only one line.

  7. Show all issues

    It's probably better to format this with one call per line:

    this.$ghostBookmark
        .removeClass('open')
        .find('.bookmark-fold')
        .css('width', 0);
    
  8. Show all issues

    This stuff should probably happen in render rather than initialize.

  9. Show all issues

    Make sure you're indenting 4 spaces in JavaScript code.

  10. Show all issues

    Make sure you're indenting 4 spaces in JavaScript code.

  11. Show all issues

    The ?: operator will already convert to a bool, so you don't need !!

  12. Show all issues

    You can use _.bind here, too.

  13. 
      
YN
YN
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
YN
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
YN
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
brennie
  1. This is looking good and a lot of these issues are style nits. However, I have some general comments:

    1. You should use sentence casing for all comments. That means they should begin with a capital and end with a period.
    2. You should prefer === over == unless you are intentionally doing a type-coercing comparision. In that case, you should document why you are using coercion over expicit equality.
    3. Your functions need doc comments unless they're overriding functions like render, initialize, etc.
  2. Show all issues

    This should be the last line in the file.

    This is a vim modeline which gives the editor settings for editing the file. Vim expects it to occur at the end of the file.

  3. Show all issues

    You can nest this in the .close rule with &:hover.

  4. Show all issues

    You should use .transition(). It does a prefixed version of transition.

  5. Show all issues

    "the diffviewer"

    Needs a period.

  6. Show all issues

    I'm not sure if the cookie data should be passed in via options or not. You should maybe ping Christian and ask him.

  7. Show all issues

    Put these on seperate lines.

  8. Show all issues

    Remove this comment.

  9. Show all issues

    We use jquery.cookie. You can use it for reading/writing cookies. Check out the docs

  10. Show all issues

    Can we call this cookie ?

  11. Show all issues

    Blank line between these.

  12. Show all issues

    Remove this comment.

  13. reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
    Show all issues

    Replace this with a for loop.

  14. reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Since we're not actually transmuting the models or options, I feel like we should do this in a signal handler.

    In initialize we can do:

    this.listenTo(this, 'update', this.saveToCookie);
    

    That way, saveToCookie will be called whenever add/remove/set is called and we don't need to define these functions.

    (Just make sure this is in initialize after the cookie data is loaded).

  15. Show all issues

    Needs a doc comment.

  16. Show all issues

    Put these on seperate lines, with defined variables first.

  17. Show all issues

    Can we call this bookmark ?

  18. Show all issues

    remove this comment.

  19. Show all issues

    This overwrites all of the cookie that is set, doesn't it? We really should use $.cookie here.

  20. Show all issues

    Needs a period.

  21. Show all issues

    Needs a doc comment.

  22. Show all issues

    Just call it template. That is the convention we use.

  23. Show all issues

    Space after , between items.

  24. Show all issues

    Remove this comment.

  25. Show all issues

    This should use sentence casing, e.g., begin with a capital and end with a period.

    "from a clone of the"

  26. Show all issues

    Missing a var statement.

    e isn't a descriptive variable name; we should use bookmark instead.

  27. reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Since you're doing a filter & map on the values and only using them once, we can just do a map and do the filtering inline as an if:

    this.collection.each(function(bookmark) {
        var fileDiffID = bookmark.get('fileDiffID'),
            interFileDiffID = bookmark.get('interFileDiffID');
    
        if (this.pageView.showsDiffFile(fileDiffID, interFileDiffID)) {
            this.createBookMarkView(bookmark, true);
        }
    }, this);
    

    This will be more efficient becuase there will be O(n) functions calls instead of ~2n.

  28. Show all issues

    Backbone.Model.get only takes one key at a time, so this should be:

    e.get('fileDiffID'), e.get('interFileDiffID')
    
  29. Show all issues

    If this is private it should be prefixed with an underscore.

    Also, this needs a doccomment.

  30. Show all issues

    Remove this line.

  31. Show all issues

    view is more readable than bview.

  32. Show all issues

    We format multi-line comments in JS as follows:

    /*
     * Line 1.
     * Line 2.
     * ...
     */
    
  33. Show all issues

    You can do view.remove().render() because remove should return the view.

  34. Show all issues

    Same comment as above regards to public/private-ness and doc comment.

  35. Show all issues

    Should be prefixed with underscore. Needs doc comment.

  36. Show all issues

    Should be prefixed with underscore. Needs doc comment.

  37. Show all issues

    Blank line between these.

  38. Show all issues

    Doc comment.

  39. Show all issues

    var $target = $(e.target),
        fileName,
        line,
        top;
    
  40. Show all issues

    Blank line between statement and block.

  41. Show all issues

    See above re: multi-line comments.

  42. Show all issues

    This is quite hard to read.

    I'm not sure what nodeType == 3 means? Is there a constnat we can use.

    Really, I'm not sure what this is doing in general. Ping me in slack and we can discuss this.

  43. Show all issues

    Should be indented 4 spaces not 2.

  44. Show all issues

    What do you mean, without updating obj?

  45. Show all issues

    Sentence casing.

  46. Show all issues

    } else {

  47. Show all issues

    doc comment.

  48. Show all issues

    Doc comment.

  49. Show all issues

    We prefix jQuery objects, so this should be $diffViewElem.

    Also, you can shorten Elem to El.

  50. Show all issues

    Needs doc comment.

  51. reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
    Show all issues

    Should be indented 4.

  52. Show all issues

    var fileDiff = file.get('filediff'),
        interFileDiff = file.get('interfilediff');
    
  53. Show all issues

    We can write this more clearly as :

    return (filediff.id === fileDiffID &&
            ((interFileDiff === null && interFileDiffID === null) ||
             interFileDiff.id === interFileDiffID));
    

    This should use === not ==.

  54. Show all issues

    ===

  55. reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6)
     
     
     
     
     
     
     
    Show all issues

    Indent multiples of 4 spaces.

  56. Show all issues

    Missing a period.

  57. Show all issues

    Missing a period.

  58. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/diffviewer/views.py (Diff revision 8)
     
     
    Show all issues
    Col: 27
     W601 .has_key() is deprecated, use 'in'
    
  3. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 9)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 10)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 11)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
chipx86
  1. First off, this feature is awesome and boy do I wish I had it while reviewing this change :)

    Second, I have a lot of comments here. The good news is, most of these are stylistic things that I repeat over and over. You'll be able to plow through most of these.

    However, I did not highlight every occurrence, so I'll want you to go through and find, for instance, cases where you have function(){ instead of function() {, and comments that need a blank line before them, etc.

    There also needs to be more documentation. We're working on fleshing these out, and we want them to follow the Python standards. I'm happy to answer questions on this.

    I also have some implementation comments, and comments about performance. For instance, I point out several cases where .parents() is used, which is an expensive function that should largely be avoided if at all possible, so I'd like to work with you to better understand these and see if we can come up with a better method.

  2. reviewboard/diffviewer/views.py (Diff revision 11)
     
     
    Show all issues

    Can we rename the cookie to "diff-bookmarks" ? Or maybe we should just have it be "bookmarks" (as we may later want to port this to text-based file attachments, perhaps).

    It's also cheaper to fetch the data out, and set the header if that data is not None. Further cheaper if you assume it exists in a try/except, like:

    try:
        response['X-ReviewBoard-Bookmarks'] = request.COOKIES['bookmarks']
    except KeyError:
        # There aren't any bookmark cookies set. Ignore.
    
  3. reviewboard/diffviewer/views.py (Diff revision 11)
     
     
     
     
    Show all issues

    Just some tweaks to this. The argument name should include the type, and for request (a very common argument), we don't need to go into details of how it'll be used. It just runs the risk of going stale as we use more of it down the road.

    request (django.http.HttpRequest):
        The HTTP request from the client.
    
  4. reviewboard/diffviewer/views.py (Diff revision 11)
     
     
     
     
     
    Show all issues

    This will be faster as:

    bookmark_cookie = request.COOKIES.get('diffbookmarks', '')
    
  5. reviewboard/static/rb/css/defs.less (Diff revision 11)
     
     
     
    Show all issues

    We namespace these names, so these would be best as @bookmark-ghost-bg-color and @bookmark-default-bg-color, in alphabetical order.

  6. Show all issues

    "Bookmarks"

  7. Show all issues

    I know we don't have this in many places, but I'm trying to get in the habit: Can you add a comment for each of these selectors documenting what part of the UI this is?

  8. Show all issues

    We always use the long form: #FFFF00. Should also have a constant.

  9. Show all issues

    All z-indexes must be registered in defs.less as constants.

  10. Show all issues

    Not sure what e() is, but elsewhere, we do:

    .transition(padding-left 0.2s linear, top 0.2s ease-out;);
    

    (Note the inner ; near the end. This forces LessCSS to consider our commas as literal text, and not argument separators.)

  11. Show all issues

    We use the form of:

    1px #333 solid

    This also needs a constant defined. Same with all other colors.

  12. Show all issues

    All repeating sizes for padding, fonts, etc. should have a constant.

  13. Show all issues

    Here and elsewhere, make sure there's a space before {.

  14. Show all issues

    Selector groups are always in this order:

    & { ... }
    &#id { ... }
    &:pseudo-class { ... }
    &.class-name { ...}
    element { ... }
    #id { ... }
    .class-name { ... }
    

    Those should be in alphabetical order within a grouping.

    This applies to all the CSS in this change.

  15. Show all issues

    No capitalization of "bookmarks" needed.

    Can you have this go into detail on what the class does? What it's tracking, how it's used, etc.

  16. Show all issues

    Multi-line comments must always be in this form:

    /*
     * line
     * line
     */
    

    Same with other comments below.

  17. Show all issues

    Function/method docs must be in this form:

    /*
     * One-line summary.
     *
     * Multi-line description.
     */
    

    When there are arguments or return values, add:

    /*
     * ...
     *
     * Args:
     *     bookmark (the type):
     *         The description.
     *
     *     next_argument (type):
     *         ...
     *
     * Returns:
     *     the return type:
     *     The description.
     */
    

    Same with all other methods.

  18. Show all issues

    Blank line between these.

  19. Show all issues

    Blank line between these.

  20. Show all issues

    Space before {.

  21. Show all issues

    Let's pull out model.get('fileDiffID') just once.

  22. Show all issues

    This would be better named as "updatedCookie".

    Make sure this is documented in the method docs.

  23. Show all issues

    No blank line here.

  24. Show all issues

    We only use one var statement per function, with one variable per line, like:

    var collection,
        listenerObj;
    
  25. Show all issues

    I don't know that this is really needed. If you just need to check that the event emitted properly, you should spy on ollection.trigger and check that it was called with the expected parameters for emitting that event.

  26. Show all issues

    Space before {.

    No need for return;, since that's implied.

  27. Show all issues

    Ideally, you should use .toBe instead of .toEqual in tests. The reason is that .toBe is equivalent to strict JavaScript comparison (===), which means that 1 === 1 but 1 !== "1".

    If you use .toEqual, it's like ==, so 1 == "1".

    Unit tests should be strict, so we want to catch future issues like the possibility of an integer turning into a string.

  28. Show all issues

    Can you add docs for each of these?

  29. Show all issues

    Just like methods, this should be in the form of:

    /*
     * One-line summary.
     *
     * Multi-line description.
     */
    

    This should also go into more detail of what this class is in charge of doing.

  30. Show all issues

    This is the default, so you don't need this here.

  31. Show all issues

    When named template, it implies it's the template for the view itself. You probably want to call this ghostBookmarkTemplate.

  32. Show all issues

    The strings must align. Indentation should be within the string, 1 space.

  33. Show all issues

    Each method needs a docstring.

  34. Show all issues

    Element-related initialization should go in render.

  35. Show all issues

    Any member variables that don't need public access should be made private (prefix the name with a _).

  36. Show all issues

    render must return this at the end.

  37. Show all issues

    render shouldn't be responsible for deciding where its main element is put. That's the responsibility of the caller, only.

  38. Show all issues

    These should instead go in events, like:

    events: {
        'click .ghost': '_onGhostBookmarkClicked',
        ...
    }
    
    1. $ghostBookmark is actually not a child of the container $el, so event listeners need to be registered manually.

  39. Show all issues

    That sounds odd to me, but you should definitely be doing:

    return _super(this).remove.call(this);
    

    instead of removing it manually. The parent remove() does more than this.

  40. Show all issues

    Can you tell me what the body query is about? Is this about whether it's currently packed into the page somewhere?

  41. Show all issues

    What's this for? Ideally, you wouldn't need to do this. I want to be sure I understand it well.

  42. Show all issues

    The list of variables should begin with defined variables, so:

    var cookie = JSON.parse(cookieString),
        bookmark;
    
  43. Show all issues

    i must be declared above, in the existing var list.

  44. Show all issues

    All jQuery-wrapped elememnts should be named like $diffReviewable.

  45. Show all issues

    This is a bit confusing. Can you instead just do this work when binding the element? If you're just looking to bind additional arguments, you can pass them to _.bind().

  46. Show all issues

    One var statement.

  47. Show all issues

    .parents() is very expensive. Is there any way we can be smarter about what we're looing up? Can you explain these checks in more detail to me?

    I'm wondering if hasClass() would be enough.

  48. Show all issues

    Blank line after this.

  49. Show all issues

    This can be one statement, and you can probably use .width(). How about:

    var $bookmarkText = this._$ghostBookmark.find('.bookmark-text');
    
    this._$ghostBookmark.find('.bookmark-fold')
        .width($bookmarkText.outerWidth());
    

    Also, how about pulling out things like .bookmark-text in render() above, so that we don't have to keep re-fetching?

  50. Show all issues

    Sentence capitalization and period.

  51. Show all issues

    The .css should be indented one.

  52. Show all issues

    These should go first, since they're defined.

  53. Show all issues

    .parents() is very expensive, so we shoud minimize this however we can. Certainly, we should never call it more than once for the same element with the same selector.

  54. reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
     
     
     
     
     
     
     
     
     
    Show all issues

    This is very awkwardly formatted. can we pull some of this out somehow?

  55. Show all issues

    This has already been pulled out of diffModel.

  56. Show all issues

    Only one blank line here.

  57. Show all issues

    Space before {.

  58. Show all issues

    filename +=.

  59. Show all issues

    You can chain these.

  60. Show all issues

    Blank line between these.

  61. Show all issues

    Values should align.

  62. Show all issues

    Blank line between these.

  63. Show all issues

    Needs docs.

  64. reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11)
     
     
     
     
     
     
     
     
    Show all issues

    Same comments as above with the formatting.

  65. Show all issues

    This will cause problems. You need to instead do:

    this.$el.html(this.template());
    

    Also, this must happen in render().

  66. Show all issues

    Anything that isn't public API should have a _ in front of it.

  67. Show all issues

    Blank line before comments.

  68. Show all issues

    Would be best to pull these out into variables, since other things seem to use these.

  69. Show all issues

    Blank lines before comments.

  70. Show all issues

    If it's just one property, do:

    this.$el.css('top', topPosition);
    
  71. Show all issues

    Blank line between these.

    vars and returns generally should be thought of as their own "paragraphs" of code.

  72. Show all issues

    Parameters must align.

  73. Show all issues

    Blank line between these.

  74. Show all issues

    Need a space before {.

  75. Show all issues

    Because of how JavaScript works, this can pick up data you don't expect. You need to use this form:

    for (var fileID in data) {
        if (data.hasOwnProperty(fileID)) {
            ...
        }
    }
    

    That assumes that data is an object and not an array.

    Also, space before {.

  76. Show all issues

    Best to only pull this out once.

  77. Show all issues

    Indentation is wrong.

    !== should be used instead of !=.

  78. reviewboard/staticbundles.py (Diff revision 11)
     
     
     
    Show all issues

    Should ideally be in alphabetical order.

  79. reviewboard/staticbundles.py (Diff revision 11)
     
     
    Show all issues

    Here too.

  80. reviewboard/staticbundles.py (Diff revision 11)
     
     
     
    Show all issues

    And these.

  81. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 12)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 13)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 14)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 15)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
david
david
Review request changed
Status:
Re-opened
Owner: