-
Notifications
You must be signed in to change notification settings - Fork 253
Fix template search path #473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
the source file path is now forwarded to the Harness object and on to the HTML generator code. Its the last argument to keep API compatibility. Ive also kept the output-relative search location to avoid anyone relying on that behavior
Thank you @ddi-acassidy for this suggested fix for #472. However, I think we need to discuss the implementation a bit.
|
I hadnt thought of that case. If template is specified in the include file then the search path needs to be different from when its in the main file. Fixing that requires parsing the include file separately instead of just concatenating it with the other file. |
I understand your reasoning, and the same argument was used when discussing solutions to the similar image relative path issue. However, a pragmatic approach was finally selected that adds folders of all input YAML files into a list of paths, by searching relative image paths relative to all folders in this list, and use the first one found. I suggest using the same approach when searching templates for consistency. |
Got it, I wasnt aware of how those two bullet points fit together to solve the problem. I'll look into updating my PR with those changes when I have time Relatedly, I put together support for Jinja templates as an alternative to the ersatz html comment template system available at the moment. It allows much more flexibility with HTML templates, like variable sized revision blocks and arbitrarily structured metadata, any interest in me cleaning that up and PRing it? |
Please verify that reusing the existing
See also the existing PR #382 for Jinja. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have in this review detailed my suggestions to reuse the existing image_paths
list that already has collected all input folders to resolve the issue with more than one input YAML file at different folders. If you agree to this (after testing several use cases), then as a separate commit, renaming image_paths
to input_folders
might be a good change to reflect the more generic usage.
@@ -70,6 +70,7 @@ class Harness: | |||
metadata: Metadata | |||
options: Options | |||
tweak: Tweak | |||
source_path: Path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source_path: Path | |
input_folders: Union[str, Path, List] |
@@ -697,7 +698,7 @@ def output( | |||
print("CSV output is not yet supported") | |||
# HTML output | |||
if "html" in fmt: | |||
generate_html_output(filename, bomlist, self.metadata, self.options) | |||
generate_html_output(filename, bomlist, self.metadata, self.options, self.source_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generate_html_output(filename, bomlist, self.metadata, self.options, self.source_path) | |
generate_html_output( | |
filename, bomlist, self.metadata, self.options, self.input_folders | |
) |
@@ -31,6 +31,7 @@ def parse( | |||
output_dir: Union[str, Path] = None, | |||
output_name: Union[None, str] = None, | |||
image_paths: Union[Path, str, List] = [], | |||
source_path = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source_path = None, |
@@ -115,6 +116,7 @@ def parse( | |||
metadata=Metadata(**yaml_data.get("metadata", {})), | |||
options=Options(**yaml_data.get("options", {})), | |||
tweak=Tweak(**yaml_data.get("tweak", {})), | |||
source_path=source_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest reusing the existing image_paths
that already has collected all input folders:
source_path=source_path | |
input_folders=image_paths, |
@@ -144,6 +144,7 @@ def wireviz(file, format, prepend, output_dir, output_name, version): | |||
output_dir=_output_dir, | |||
output_name=_output_name, | |||
image_paths=list(image_paths), | |||
source_path=file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source_path=file |
@@ -21,14 +21,19 @@ def generate_html_output( | |||
bom_list: List[List[str]], | |||
metadata: Metadata, | |||
options: Options, | |||
source: Union[str, Path] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source: Union[str, Path] = None, | |
input_folders: Union[str, Path, List] = [], |
template_search_paths = [ Path(filename).parent, Path(__file__).parent / "templates"] | ||
|
||
if source is not None: | ||
template_search_paths.insert(0, Path(source).parent) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
template_search_paths = [ Path(filename).parent, Path(__file__).parent / "templates"] | |
if source is not None: | |
template_search_paths.insert(0, Path(source).parent) |
if templatename: | ||
# if relative path to template was provided, check directory of YAML file first, fall back to built-in template directory | ||
templatefile = smart_file_resolve( | ||
f"{templatename}.html", | ||
[Path(filename).parent, Path(__file__).parent / "templates"], | ||
f"{templatename}.html", template_search_paths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember inserting from wireviz.wv_bom import make_list
in the import section above and run isort *.py
to format and sort them.
f"{templatename}.html", template_search_paths | |
f"{templatename}.html", | |
make_list(input_folders) + [Path(__file__).parent / "templates"], |
My suggestion here does not add the output folder in the list when different because I don't see any use case where that might be useful, but please argue against my suggestion.
Addresses #472
the source file path is now forwarded to the Harness object and on to the HTML generator code. Its the last argument with a default to keep API compatibility. Ive also kept the output-relative search location to avoid hurting anyone relying on that behavior