load_template searchpaths, take 2 #294
Conversation
|
Would you mind adding a test testing the MRO, please? You can probably build a subclass of the Thanks! |
|
How are you testing the MRO for the templates? |
|
We're checking the search path assembled, and ensuring that both FakeNetworkDriver and NetworkDriver's path strings are present, which implies that they were built correctly from the MRO |
If an explicit template_path is not specified, build a list of directories from the class MRO and pass them to FileSystemLoader
2 similar comments
napalm_base/helpers.py
Outdated
| if (isinstance(template_path, py23_compat.string_types) and | ||
| os.path.isdir(template_path) and os.path.isabs(template_path)): | ||
| # append driver name at the end of the custom path | ||
| print(template_path) |
napalm_base/helpers.py
Outdated
| raise IOError("Template path does not exist: {}".format(template_path)) | ||
| else: | ||
| # Search modules for template paths | ||
| for parent in cls.__class__.mro(): |
There was a problem hiding this comment.
search_path = [os.path.dirname(os.path.abspath(sys.modules[c.__module__].__file__))
for c in cls.__class__.mro() if c is not object]
Feels more legible.
|
We should probably break down this into smaller functions at some point, this method is... confusing. Anyway, I left a couple of minor comments. Feel free to discard the comprehension list suggestion, I just find them easier to read. |
|
Thanks for the catch and the suggestion. Before merge, I wanted one more call for comments on https://github.com/bewing/napalm-base/blob/13ecc398e70630dff337df14c57b69a0bde680ef/napalm_base/helpers.py#L44-L50, as it is a functionality change (Before we silently ignored bad paths, and looked in the driver's template directory. We could refactor now to just blindly trust the input, and have it be the first path passed into the FileLoader |
|
I don't like doing those sort of sanity checks, it pollutes the code and makes it harder to read and I am of the opinion that users should be responsible of using the library properly and sending a valid path if a valid path is what the function demands. However, your change at least throws an error instead of swallowing it so I am fine with the change. |
Build pathlists inside the helper function, pass them all to Jinja2.
Explicitly throw an error if the template_path specified is not found (Discussion needed. Silently ignore and append normal search paths instead?)