Issue 2002 - Bug with linkify js

Review Request #3374 — Created Sept. 29, 2012 and submitted

Information

Review Board
1.7.x Beta 3

Reviewers

Issue 2002 - Bug with linkify js

This further fixes the bug with regards to linkifying text. Now, URLs with anchors (open and close parens, with or without text in between them) work properly. This fix doesn't affect the behaviour observed with url wrapped in parens.
Self tested on own environment with different types of urls
- blah blah (http://www.foo.com/)
- http://download.oracle.com/javase/1.4.2/docs/api/java/lang/Object.html#clone()
- http://download.oracle.com/javase/1.4.2/docs/api/java/lang/Object.html#equals(java.lang.Object)
- gwegdss http://www.foo.com/

Description From Last Updated

var declaration in javascript should follow this style: var a = '', b = '';

ME medanat

All of these lines shown in reviewboard with big red blocks have trailing whitespace. Please clean this up.

daviddavid

Remove trailing white space (the red lines).

ME medanat

No need for two new lines here. Should be just 1.

ME medanat

Let's just include this description in the above comment. Explain there that we'll prevent it from being part of the …

daviddavid

While this comment is relevant, you shouldn't include the bug author in the comment. Just explain what this piece of …

ME medanat

By "paren", do you mean "parentheses"? If so, please use the full word for clarity.

ME medanat

This declaration could be declared by the vars 'extra' and 'parts';

ME medanat

A few trailing spaces.

ME medanat
david
  1. 
      
  2. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
    Show all issues
    All of these lines shown in reviewboard with big red blocks have trailing whitespace. Please clean this up.
  3. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    Let's just include this description in the above comment. Explain there that we'll prevent it from being part of the URL, but only if there are no open parenthesis characters in the URL.
  4. 
      
ME
  1. A few style issues I ran into.
  2. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
     
    Show all issues
    var declaration in javascript should follow this style:
    
    var a = '',
        b = '';
  3. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Remove trailing white space (the red lines).
  4. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
     
    Show all issues
    No need for two new lines here. Should be just 1.
  5. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    While this comment is relevant, you shouldn't include the bug author in the comment. Just explain what this piece of code is doing or what purpose it's serving.
  6. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
    Show all issues
    By "paren", do you mean "parentheses"? If so, please use the full word for clarity.
  7. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
    Show all issues
    This declaration could be declared by the vars 'extra' and 'parts';
  8. 
      
JS
ME
  1. Well done! Remove the last trailing spaces and you're set!
    
    One thing I always do is check the diff viewer before submitting a review to see if there are trailing spaces or other obvious issues.
    1. You should also mark issues as "fixed" once fixed.
  2. reviewboard/static/rb/js/reviews.js (Diff revision 2)
     
     
     
     
     
     
     
     
    Show all issues
    A few trailing spaces.
    1. Thanks Yazan! I will definitely need to find a way to look for trail space. Do you have any recommendations to detect them before committing and having to view a diff?
      
      Perhaps my IDE could do it. I'll google this.
      
      
    2. If you're using vim, here's what I have in my .vimrc:
      
      
      " Highlight trailing whitespace
      autocmd BufNewFile,BufReadPost,WinEnter * highlight WhitespaceEOL ctermbg=red guibg=red
      autocmd BufNewFile,BufReadPost,WinEnter * match WhitespaceEOL /\s\+$/
      
    3. Most text editors (at least the ones I have used) tend to have an option to remove trailing spaces on save or something of the sort.
      
      It's still a good idea to go over your review's diff before submitting. This helps you make sure that it includes the changes you intended to make (and only your changes, not some other person's changes). It also helps you make sure your logic/style is correct and you haven't left any print or debug statements in the review.
      
      Hope that helps!
  3. 
      
JS
ME
  1. Ship It!
  2. 
      
david
  1. Ship It!
  2. 
      
JS
Review request changed
Status:
Completed
Change Summary:
Pushed to master (28878fc). Thanks!