Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ddi-acassidy
Copy link

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

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
@kvid
Copy link
Collaborator

kvid commented Jul 25, 2025

Thank you @ddi-acassidy for this suggested fix for #472. However, I think we need to discuss the implementation a bit.

  1. It seems to fix the issue for simple cases where the template name is specified in the main YAML input file, but not if the template name is specified in a prepend YAML file located in a different folder than the main YAML input file.
  2. The problem described above is very similar to searching for images with a relative path, and I think we can reuse the image_paths list that was created to resolve the same issue for images with a relative path. If this can be verified, then I suggest renaming it to input_paths and use it for all relative paths (to images, templates, and any other separate input files specified by a relative path).
  3. If my suggestion is accepted and verified, then PR Add support for a custom template directory path #444 also needs to be adjusted accordingly.
  4. Please argue against my suggestion, and search for a use case where my suggestion will not be a good solution.
  5. Please remember running isort and black on Python files before committing your changes - as described together with other recommendations in CONTRIBUTING.md.

@ddi-acassidy
Copy link
Author

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.

@kvid
Copy link
Collaborator

kvid commented Jul 25, 2025

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.

@ddi-acassidy
Copy link
Author

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?

@kvid
Copy link
Collaborator

kvid commented Jul 26, 2025

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

Please verify that reusing the existing image_paths variable will resolve the issue before (in a separate commit) renaming the variable to a generic name.

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?

See also the existing PR #382 for Jinja.

Copy link
Collaborator

@kvid kvid left a 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Collaborator

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:

Suggested change
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
source: Union[str, Path] = None,
input_folders: Union[str, Path, List] = [],

Comment on lines +28 to +32
template_search_paths = [ Path(filename).parent, Path(__file__).parent / "templates"]

if source is not None:
template_search_paths.insert(0, Path(source).parent)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Collaborator

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.

Suggested change
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants