code review 7312043: codereview: add support for linking to Google Code issu...

codereview: add support for linking to Google Code issue tracker

If a description includes the line
    REPO=code.google.com/p/whatever
then throughout that description the text 'issue nnn' becomes
a link to issue nnn on that Google Code project.
The final REPO= line wins, so the one below overrides the
one above.

Also, the phrase 'CL nnnnn' anywhere turns into a link to
that page on the current code review server.
For example, this is CL 7312043.

Running at rsc-dot-codereview.appspot.com.

Fixes issue 422.

REPO=code.google.com/p/rietveld

[rsc, 2013-02-05 14:28:11]

Hello maruel@chromium.org,

I'd like you to review this change to
https://code.google.com/p/rietveld/

[rsc, 2013-02-05 14:29:17]

See the description at http://rsc-dot-codereview.appspot.com/7312043 for a
demo. The idea is that tools that generate code reviews would include the
REPO= line automatically.

Russ

[M-A, 2013-02-07 18:17:30]

Sorry for the long delay; was busy breaking other projects.

https://codereview.appspot.com/7312043/diff/3004/codereview/library.py
File codereview/library.py (right):

https://codereview.appspot.com/7312043/diff/3004/codereview/library.py#newcod...
codereview/library.py:143: for i in range(len(chunks)):
for i, chunk in enumerate(chunks):
would remove the need for line 144.

https://codereview.appspot.com/7312043/diff/3004/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/7312043/diff/3004/codereview/models.py#newcode199
codereview/models.py:199: """Parses a description, returning automatic hyperlink
rewrites to be applied.
80 cols

https://codereview.appspot.com/7312043/diff/3004/codereview/models.py#newcode228
codereview/models.py:228: (r'(?i)\b(issue\s+([0-9]+))\b',
In chromium, we use BUG=<number>

https://code.google.com/p/rietveld/source/browse/codereview/views.py?name=chr...

It was not ported back because we weren't sure it was valuable. In particular,
it uses the format:
BUG=<project_name>:<issue_number>
to be able to reference arbitrary project, for example chromium-os bugs on the
chromium code review. The main reason it wasn't ported back is that
<project_name> is optional and defaults to "chromium" in the chromium branch.
Also I think your code is cleaner, I didn't write the chromium's version, so
it's probably worth doing a switch over anyway.

I like standardizing on the "([A-Z]+)\s*=\s*(.*)" form for its simplicity; it's
clear that the line could be processed automatically, unlike more free form text
like "CL 123". But I'm not against "CL 123" form, just noting the inherent
difference. In fact, I think it's nicer than the current url copy-paste we
generally do. But I'd probably prefer something like
DEPENDSON=<issue_number>

and 'urlize' that.

The other thing I'm unsure is that you add html tags without escaping first.
Does this function expect |description| to be escaped first?

On your test instance, the REPO= line doesn't get transformed to an url.

https://codereview.appspot.com/7312043/diff/3004/templates/issue.html
File templates/issue.html (right):

https://codereview.appspot.com/7312043/diff/3004/templates/issue.html#newcode25
templates/issue.html:25:
<pre>{{issue.description|wordwrap:80|urlizetrunc:80|issue_autolink}}</pre>
I feel you would need to |escape before creating the autolink?

[rsc, 2013-02-08 04:09:38]

Will apply the code fixes in the morning, just wanted to reply to the
substantive things. 

Let me know if you'd like me to add support for BUG= too.

https://codereview.appspot.com/7312043/diff/3004/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/7312043/diff/3004/codereview/models.py#newcode228
codereview/models.py:228: (r'(?i)\b(issue\s+([0-9]+))\b',
I'm happy to add support for BUG= too, but the 'issue NNN' is important to Go
and I assume to any project using the Google Code issue tracker: it matches the
syntax used in CL directives like "Fixes issue nnn." or "Update issue nnn."

The REPO= line doesn't get transformed because the prefix makes it something
that Django's urlize will ignore. We could handle it specially I guess.

https://codereview.appspot.com/7312043/diff/3004/templates/issue.html
File templates/issue.html (right):

https://codereview.appspot.com/7312043/diff/3004/templates/issue.html#newcode25
templates/issue.html:25:
<pre>{{issue.description|wordwrap:80|urlizetrunc:80|issue_autolink}}</pre>
On 2013/02/07 18:17:30, M-A wrote:
> I feel you would need to |escape before creating the autolink?

urlizetrunc does the escaping: it takes text and generates HTML where the text
is escaped and some things that look like URLs are now links. The trunc:80 part
means that very long links have their link text truncated to 80 chars + "...".

So the input to issue_autolink is HTML, and the output is similarly HTML.

[M-A, 2013-02-08 19:52:51]

Added the mailing list, in particular to know if anyone would like to have
support for the format BUG=<project>:<number>.

lgtm

https://codereview.appspot.com/7312043/diff/3004/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/7312043/diff/3004/codereview/models.py#newcode228
codereview/models.py:228: (r'(?i)\b(issue\s+([0-9]+))\b',
On 2013/02/08 04:09:38, rsc wrote:
> I'm happy to add support for BUG= too, but the 'issue NNN' is important to Go
> and I assume to any project using the Google Code issue tracker: it matches
the
> syntax used in CL directives like "Fixes issue nnn." or "Update issue nnn."

Ok I don't mind. I don't know if others would like BUG=, if nobody raises his
hand, no need to implement.
 
> The REPO= line doesn't get transformed because the prefix makes it something
> that Django's urlize will ignore. We could handle it specially I guess.

It's up to you, we don't use this format.

https://codereview.appspot.com/7312043/diff/3004/templates/issue.html
File templates/issue.html (right):

https://codereview.appspot.com/7312043/diff/3004/templates/issue.html#newcode25
templates/issue.html:25:
<pre>{{issue.description|wordwrap:80|urlizetrunc:80|issue_autolink}}</pre>
On 2013/02/08 04:09:38, rsc wrote:
> On 2013/02/07 18:17:30, M-A wrote:
> > I feel you would need to |escape before creating the autolink?
> 
> urlizetrunc does the escaping: it takes text and generates HTML where the text
> is escaped and some things that look like URLs are now links.

Thanks, I was confused.