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. |
kvid
left a comment
There was a problem hiding this comment.
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.
| metadata: Metadata | ||
| options: Options | ||
| tweak: Tweak | ||
| source_path: Path |
There was a problem hiding this comment.
| source_path: Path | |
| input_folders: Union[str, Path, List] |
| # 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.
| generate_html_output(filename, bomlist, self.metadata, self.options, self.source_path) | |
| generate_html_output( | |
| filename, bomlist, self.metadata, self.options, self.input_folders | |
| ) |
The splitting into multiple lines is due to black formatting.
| 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.
| source_path = None, |
| 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.
I suggest reusing the existing image_paths that already has collected all input folders:
| source_path=source_path | |
| input_folders=image_paths, |
| output_dir=_output_dir, | ||
| output_name=_output_name, | ||
| image_paths=list(image_paths), | ||
| source_path=file |
There was a problem hiding this comment.
| source_path=file |
| bom_list: List[List[str]], | ||
| metadata: Metadata, | ||
| options: Options, | ||
| source: Union[str, Path] = None, |
There was a problem hiding this comment.
| 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.
| template_search_paths = [ Path(filename).parent, Path(__file__).parent / "templates"] | |
| if source is not None: | |
| template_search_paths.insert(0, Path(source).parent) |
| 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.
Remember inserting from wireviz.wv_bom import make_list in the import section above and run isort *.py to format and sort them. (Github don't allow me to directly suggest changes at lines more than a few lines away from existing PR changes.)
| 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.
|
Hi, |
|
@tbornon-sts wrote:
As you can see above, I have reviewed this suggested change and verified that the simple cases are resolved, but I suggested a few more changes to also resolve the more general cases. Feel free to download/install the suggested changes from this branch yourself to test if your use case is resolved. If the author doesn't find time to do the work, then we probably should consider merging it as is and add the additional changes on top. |
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