Summary

Issue 2002 - Bug with linkify js

Review Request #3374 — Created Sept. 29, 2012 and submitted Sept. 29, 2012, 10:13 a.m.

Information

Owner

jsintal

Repository

Review Board

Branch

1.7.x Beta 3

Bugs

2002

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

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/

Issues

Description	From	Last Updated
var declaration in javascript should follow this style: var a = '', b = '';	ME medanat	Sept. 29, 2012, 8:38 a.m.
All of these lines shown in reviewboard with big red blocks have trailing whitespace. Please clean this up.	david	Sept. 29, 2012, 8:38 a.m.
Remove trailing white space (the red lines).	ME medanat	Sept. 29, 2012, 8:38 a.m.
No need for two new lines here. Should be just 1.	ME medanat	Sept. 29, 2012, 8:38 a.m.
Let's just include this description in the above comment. Explain there that we'll prevent it from being part of the …	david	Sept. 29, 2012, 8:38 a.m.
While this comment is relevant, you shouldn't include the bug author in the comment. Just explain what this piece of …	ME medanat	Sept. 29, 2012, 8:38 a.m.
By "paren", do you mean "parentheses"? If so, please use the full word for clarity.	ME medanat	Sept. 29, 2012, 8:38 a.m.
This declaration could be declared by the vars 'extra' and 'parts';	ME medanat	Sept. 29, 2012, 8:38 a.m.
A few trailing spaces.	ME medanat	Sept. 29, 2012, 8:42 a.m.

reviewboard/static/rb/js/reviews.js (Diff revision 1)

The issue has been resolved. Show all issues

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

reviewboard/static/rb/js/reviews.js (Diff revision 1)

The issue has been resolved. 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.

```
A few style issues I ran into.
```
reviewboard/static/rb/js/reviews.js (Diff revision 1)
The issue has been resolved. Show all issues
```
var declaration in javascript should follow this style:

var a = '',
    b = '';
```

reviewboard/static/rb/js/reviews.js (Diff revision 1)

The issue has been resolved. Show all issues

Remove trailing white space (the red lines).

reviewboard/static/rb/js/reviews.js (Diff revision 1)
The issue has been resolved. Show all issues
```
No need for two new lines here. Should be just 1.
```

reviewboard/static/rb/js/reviews.js (Diff revision 1)

The issue has been resolved. 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.

reviewboard/static/rb/js/reviews.js (Diff revision 1)
The issue has been resolved. Show all issues
```
By "paren", do you mean "parentheses"? If so, please use the full word for clarity.
```
reviewboard/static/rb/js/reviews.js (Diff revision 1)
The issue has been resolved. Show all issues
```
This declaration could be declared by the vars 'extra' and 'parts';
```

Change Summary:

Fixed only certain style and commenting issues.

Diff:

Revision 2 (+9 -5)

Show changes

reviewboard/static/rb/js/reviews.js

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.

medanat Sept. 29, 2012, 8:29 a.m.

You should also mark issues as "fixed" once fixed.

reviewboard/static/rb/js/reviews.js (Diff revision 2)

The issue has been resolved. Show all issues

A few trailing spaces.

jsintal Sept. 29, 2012, 9:07 a.m.

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.

david Sept. 29, 2012, 9:08 a.m.

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\+$/

medanat Sept. 29, 2012, 9:16 a.m.

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!

Change Summary:

Style fixes.

Diff:

Revision 3 (+8 -4)

Show changes

reviewboard/static/rb/js/reviews.js

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (28878fc). Thanks!