Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29327966: Issue 3084 - [cms] Show full tracebacks for exceptions passing template code (Closed)

Created:
Sept. 15, 2015, 5:37 p.m. by Sebastian Noack
Modified:
Sept. 17, 2015, 9:30 p.m.
Reviewers:
kzar, Wladimir Palant
Visibility:
Public.

Patch Set 1 #

Total comments: 6

Patch Set 2 : Pass through filename and use execfile() when loading filters/globals #

Total comments: 11

Patch Set 3 : Addressed comments #

Total comments: 6

Patch Set 4 : Unpack converter source before calling get_html() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+61 lines, -41 lines) Patch
M cms/bin/generate_static_pages.py View 1 1 chunk +1 line, -1 line 0 comments Download
M cms/converters.py View 1 2 3 6 chunks +39 lines, -22 lines 0 comments Download
M cms/sources.py View 1 2 7 chunks +21 lines, -18 lines 0 comments Download

Messages

Total messages: 12
Sebastian Noack
https://codereview.adblockplus.org/29327966/diff/29327967/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29327966/diff/29327967/cms/converters.py#newcode125 cms/converters.py:125: for i, line in enumerate(lines): In order to prevent ...
Sept. 15, 2015, 5:48 p.m. (2015-09-15 17:48:37 UTC) #1
Wladimir Palant
IMHO, whoever is calling the converter should set the pagefile parameter in addition to pagedata ...
Sept. 15, 2015, 6:37 p.m. (2015-09-15 18:37:57 UTC) #2
Wladimir Palant
Alternatively, params["pagedata"] could be a dictionary instead of a string: {"filename": "foo.md", "contents": "Hello, World!"}.
Sept. 15, 2015, 6:39 p.m. (2015-09-15 18:39:19 UTC) #3
Sebastian Noack
New patch set is up. Indeed, passing through the filename from Source.read_file() all the way ...
Sept. 16, 2015, 11:20 a.m. (2015-09-16 11:20:58 UTC) #4
Wladimir Palant
https://codereview.adblockplus.org/29327966/diff/29327997/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29327966/diff/29327997/cms/converters.py#newcode131 cms/converters.py:131: lines[i] = self.removed_line Why not just assign "\n" here? ...
Sept. 16, 2015, 5:35 p.m. (2015-09-16 17:35:53 UTC) #5
Sebastian Noack
https://codereview.adblockplus.org/29327966/diff/29327997/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29327966/diff/29327997/cms/converters.py#newcode131 cms/converters.py:131: lines[i] = self.removed_line On 2015/09/16 17:35:52, Wladimir Palant wrote: ...
Sept. 16, 2015, 7:04 p.m. (2015-09-16 19:04:05 UTC) #6
Wladimir Palant
LGTM https://codereview.adblockplus.org/29327966/diff/29327997/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29327966/diff/29327997/cms/sources.py#newcode232 cms/sources.py:232: return (data, None) On 2015/09/16 19:04:04, Sebastian Noack ...
Sept. 16, 2015, 9:08 p.m. (2015-09-16 21:08:59 UTC) #7
kzar
(Just being nosy in the hope to learn something, I realise this has already been ...
Sept. 17, 2015, 8:41 a.m. (2015-09-17 08:41:18 UTC) #8
Sebastian Noack
https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py#newcode284 cms/converters.py:284: def get_html(self, (source, filename)): On 2015/09/17 08:41:17, kzar wrote: ...
Sept. 17, 2015, 8:50 a.m. (2015-09-17 08:50:00 UTC) #9
Sebastian Noack
https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py#newcode284 cms/converters.py:284: def get_html(self, (source, filename)): On 2015/09/17 08:49:59, Sebastian Noack ...
Sept. 17, 2015, 10:01 a.m. (2015-09-17 10:01:23 UTC) #10
kzar
LGTM
Sept. 17, 2015, 10:02 a.m. (2015-09-17 10:02:58 UTC) #11
Wladimir Palant
Sept. 17, 2015, 9:30 p.m. (2015-09-17 21:30:14 UTC) #12
Message was sent while issue was closed.
LGTM again

https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py#n...
cms/converters.py:373: template = jinja2.Template.from_code(env, code,
env.globals)
On 2015/09/17 08:49:59, Sebastian Noack wrote:
> Indeed, it's not documented, at least not in the online docs.

It's documented under
http://code.nabla.net/doc/jinja2/api/jinja2/environment/jinja2.environment.Te...
but apparently that's not the official documentation.

Powered by Google App Engine
This is Rietveld