reStructuredText rendering errors don't show filename, just "string"

[matt-garman]

If my reStructuredText source document has an error, the filename is not printed, only "string". For example:

<string>:29: (ERROR/3) Unexpected indentation.

I'm not sure if this happens with other renderers too, I currently only use RST.

In renderers.py, for the ReStructuredText implementation, there is a call to docutils.core.publish_parts() (line 92). This function takes an optional parameter, "source_path", which is currently not used by Wok. Whatever this is set to will be displayed when there is an error, instead of the ambiguous "string" (as shown above). Both the documentation and my experimentation suggest this variable is for display purposes only, i.e. it doesn't actually try to reference that variable as a file.

I think this is a relatively easy fix: to the generic Renderer.render() function, I could add an additional optional variable that defaults to None. This variable would be "source_path" (or "source_filename" or even "slug"), and if set, would be passed through to the the underlying renderer if supported.

Maybe there's a better/cleaner way to do this that is less "invasive" to the API? I'm happy to work on this, but would like a little guidance on the best route to take. Thanks!

[edunham]

Thanks for the issue report! I think adding the source_path sounds like a reasonable fix.

@mythmon thoughts on that solution?

[mythmon]

This doesn't seem invasive to the API to me. I like the idea of adding the source_path argument to the render function. That's what it is there for, after all.

@matt-garman, if you'd like to file a PR for those changes, that would be awesome 🎆. Otherwise, I'm sure someone will get to it at some point.

[matt-garman]

Thank you for the quick feedback! I should be able to come up with a PR fairly soon.

Another idea: the render() function could be changed to take an object; and that object would initially contain the to-be-rendered text plus the source filename. If ever more metadata is to be passed to the underlying renderers, this might be a little cleaner (or maybe messier, depending on your perspective ;)).

Sorry, I accidentally closed this, re-opening.

[matt-garman]

This is in theory easy to implement, but I'm getting hung up on something that doesn't make sense to me. In renderers.py, I simply added a "source_path=None" argument to all render() definitions. And then in page.py, for the from_file() function, I modified the content rendering line like this:

page.meta['content'] = page.renderer.render(page.original, source_path=page.path)

It works as expected for everything in the test_site, except the html_renderer.html page, where it blows up with this:

Traceback (most recent call last):
  File "/home/matt/development/web/virtualenv/wok/bin/wok", line 5, in <module>
    pkg_resources.run_script('wok==1.1.1', 'wok')
  File "/home/matt/development/web/virtualenv/wok/lib/python2.7/site-packages/pkg_resources.py", line 540, in run_script
    self.require(requires)[0].run_script(script_name, ns)
  File "/home/matt/development/web/virtualenv/wok/lib/python2.7/site-packages/pkg_resources.py", line 1462, in run_script
    exec_(script_code, namespace, namespace)
  File "/home/matt/development/web/virtualenv/wok/lib/python2.7/site-packages/pkg_resources.py", line 41, in exec_
    exec("""exec code in globs, locs""")
  File "<string>", line 1, in <module>
  File "/home/matt/development/web/virtualenv/wok/lib/python2.7/site-packages/wok-1.1.1-py2.7.egg/EGG-INFO/scripts/wok", line 4, in <module>
    import pkg_resources
  File "build/bdist.linux-x86_64/egg/wok/engine.py", line 151, in __init__
  File "build/bdist.linux-x86_64/egg/wok/engine.py", line 189, in generate_site
  File "build/bdist.linux-x86_64/egg/wok/engine.py", line 369, in load_pages
  File "build/bdist.linux-x86_64/egg/wok/page.py", line 123, in from_file
TypeError: render() got an unexpected keyword argument 'source_path'

So just before the render() call I added some prints:

        print "DEBUG: page.path=\"" + str(page.path) + "\""
        print "DEBUG: page.renderer=\"" + str(page.renderer) + "\""
        print "DEBUG: page.renderer.render=\"" + str(page.renderer.render) + "\""

For the stuff that works, the output of these prints is predictable:

DEBUG: page.path="content/tests/dates3.mkd"
DEBUG: page.renderer="<class 'wok.renderers.Markdown'>"
DEBUG: page.renderer.render="<bound method type.render of <class 'wok.renderers$Markdown'>>"

But for the html file, it shows this:

DEBUG: page.path="content/tests/html_renderer.html"
DEBUG: page.renderer="<class '__renderers__.'>"
DEBUG: page.renderer.render="<function render at 0x15917d0>"

I don't really understand where this "function render at 0x15917d0" actually lives in the source.

Any thoughts?

Thanks!

[mythmon]

Needing to add source_path=None to all the calls seems wrong. I think it would be better to make the renderer that cares about paths have a source_path parameter that defaults to None. For calls that don't care about the source path, they can leave it out. Then you can add it back in only for the code paths that need it.

I'm not quite sure what could cause the errors you talk about though. I think it would be better to post a PR with your code in it, even if the code is unfinished. Talking about this error will be easier when we can all see the code.

Thanks for the work on this!

[matt-garman]

Needing to add source_path=None to all the calls seems wrong. I think it would be better to make the renderer that cares about paths have a source_path parameter that defaults to None. For calls that don't care about the source path, they can leave it out. Then you can add it back in only for the code paths that need it.

I agree, but I don't see how this is possible without more dramatic changes to the code.

The Renderer class (and all sub-classes) defines the render() function as static (@classmethod). Every page object has a renderer member defined by its extension. When the page.renderer.render() function is actually called, it's not known if source_path is needed or not. IOW, to use the source_path parameter at this point, the filename extension check would have to be re-done which to me also seems wrong.

Here's another idea, though it's much more substantial in scope: change render() to be non-static. Create something like a factory that will produce a Renderer object based on filename extension, on a per-page basis (i.e. each page has its own Renderer instance). The ReStructuredText Renderer class can have a source_path member which will be set when instantiated (i.e. by the factory).

I'm not quite sure what could cause the errors you talk about though. I think it would be better to post a PR with your code in it, even if the code is unfinished. Talking about this error will be easier when we can all see the code.

Done! Hopefully some of what I wrote above will make more sense in light of the PR code.

Thanks for the work on this!

You are welcome, though I feel like I'm creating more work for you! :)