|
|
|
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
Patch Set 1 #Patch Set 2 : diff -r 47341173aa45 https://code.google.com/p/rietveld/ #Patch Set 3 : diff -r 47341173aa45 https://code.google.com/p/rietveld/ #Patch Set 4 : diff -r 47341173aa45 https://code.google.com/p/rietveld/ #
Total comments: 8
Total messages: 5
|
rsc
Hello maruel@chromium.org, I'd like you to review this change to https://code.google.com/p/rietveld/
|
12 years, 11 months ago (2013年02月05日 14:28:11 UTC) #1 | ||||||||||||||||||||||||||||||||
Hello maruel@chromium.org, I'd like you to review this change to https://code.google.com/p/rietveld/
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
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?
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.
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.