Added better support to template loader for paths that don't use slash #303

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

mearns commented Feb 28, 2014

Very simple change to make a path canonical (use '/' as separator) before splitting.

Owner

mitsuhiko commented Jun 6, 2014

There are a bunch of problems with this: first it changes behavior, secondly it does not work on any platform other than windows (because os.altsep is None). Also I do not see what this fixes. You should always be using / in templates no matter what OS.

mitsuhiko closed this Jun 6, 2014

mearns commented Jun 9, 2014

Didn't realize os.altsep was None on other systems. Admittedly this was a quick hack to fix an issue I was having, but I still suggest you consider this an open bug: if you want to write system-independent code, then you should accept paths in the native format for the system. Most of the python built in functions that return a file system path use the native format (like os.getcwd for instance), and if you use these to load templates on Windows, it fails.

Owner

mitsuhiko commented Jun 9, 2014

Why would yu use os.getcwd() for loading templates though? File system paths have nothing to do with jinja template paths. Not even close :)

mearns commented Jun 10, 2014

How about FileSystemLoader? That loads templates from a directory on the file system, right? And it uses the altered function split_template_path to do so. This doesn't work correctly on Windows. So perhaps the changes were made in the wrong place, or perhaps were just done incorrectly, but I still suggest that if you have a class that works with file system paths (such as FileSystemLoader) it be able to support file-system paths in the correct format for whatever system its on.

Owner

mitsuhiko commented Jun 10, 2014

But the template name is always slash separated, no matter the loader. It was never supposed to accept file system paths and I am pretty sure it's not documented anywhere to support it.

mearns commented Jun 11, 2014

Alright, well it's you're project. From a user's perspective, I expect to be able to pass a file-system path to FileSystemLoader.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment