-
Notifications
You must be signed in to change notification settings - Fork 227
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
Improve output of diagram + BOM as technical drawing #239
Conversation
6f56fcd
to
627096f
Compare
This PR is ready for review. I have been using this feature and the included DIN 6771 template files for quite a while and I am very pleased with the results. In the interest of not delaying the merge too much, I would request to address the following minor issues as separate PRs:
|
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.
- This preliminary review is only based on inspection of changes, not testing.
- Consider also replacing all keywords from the template in one pass to avoid replacement of replacements (and to increase efficiency), e.g. like in the lead answer here: https://stackoverflow.com/questions/6116978/how-to-replace-multiple-substrings-of-a-string
- Have you considered supporting an optional default value of each keyword in templates? (e.g. something like this:
<!-- %bgcolor%white% -->
)
Thanks, I will incorporate some of your changes soon and wait for some testing.
👍
Sounds like a nice follow-up PR. |
a8731e2
to
83ab27a
Compare
Moved metadata and options info further down, so that the core functionality (connectors, cables, connection sets) comes first.
59ee742
to
b0ab711
Compare
b0ab711
to
293c896
Compare
- Use pin names instead of pin indices, until the last moment when generating the ports for the GraphViz nodes - `Harness.add_mate_pin()` now uses pin names - Remove unused `if is_arrow()` check from `Harness.connect()` - Consolidate calling of `Connector.activate_pin()` to prevent subtle bugs - Call it from `connect()` and `add_mate_pin()` - No longer call it from `create_graph()` - Misc. other tuning
Use `output_formats` parameter to specify which *files* to output to disk, Use `return_types` parameter to specify which objects to return to a calling Python script
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 like this concept of templates and the generic replacement rules. However, I have included a few more suggestions.
for possible_path in possible_paths: | ||
resolved_path = (possible_path / filename).resolve() |
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.
To improve readability slightly, I suggest the shorter path
as loop variable in this case as it is not ambiguous, and easier to distinguish from possible_paths
:
for possible_path in possible_paths: | |
resolved_path = (possible_path / filename).resolve() | |
for path in possible_paths: | |
resolved_path = (path / filename).resolve() |
} | ||
|
||
</style> | ||
</head><body style="font-family:<!-- %fontname% -->;background-color:<!-- %bgcolor% -->"> |
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.
Have you considered moving these two style entries of the body tag into a new body entry in the upper part of the style tag above, as it is in the DIN-6771 template?
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.
Or maybe these style entries are intentionally defined in two different ways in the two templates to demonstrate both ways?
src/wireviz/wv_helper.py
Outdated
if resolved_path.exists(): | ||
return resolved_path | ||
else: | ||
raise Exception(f'{filename} was not found in any of the following locations: \n' + | ||
'\n'.join([str(x) for x in possible_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.
- The function name
smart_file_resolve()
indicate that it will only resolve a regular file (or maybe a symbolic link pointing to a regular file). If that's the intention, then I suggest testingis_file()
to avoid accepting e.g. a directory with the same name, and also explain that a file with this name was not found in the exception case. - I also recommend double quotes around the filename to increase readability when the filename might contain spaces. If it's really important to avoid such quotes when the filename doesn't contain spaces, then I suggest a new helper function:
def quote_if_needed(name: str) -> str: return f'"{name}"' if ' ' in name else name
if resolved_path.exists(): | |
return resolved_path | |
else: | |
raise Exception(f'{filename} was not found in any of the following locations: \n' + | |
'\n'.join([str(x) for x in possible_paths])) | |
if resolved_path.is_file(): | |
return resolved_path | |
else: | |
raise Exception(f'A file named "{filename}" was not found in any of the following locations:\n' + | |
'\n'.join([str(x) for x in possible_paths])) |
src/wireviz/wv_html.py
Outdated
templatefile = smart_file_resolve(f'{templatename}.html', [Path(filename).parent, Path(__file__).parent / 'templates']) | ||
else: | ||
# fall back to built-in simple template if no template was provided | ||
templatefile = Path(__file__).parent / 'templates/simple.html' |
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 defining STD_TEMPLATES = Path(__file__).parent / 'templates'
in the global scope together with other global constants (consider also alternative constant names for a better fit with other constants) and use it here:
templatefile = smart_file_resolve(f'{templatename}.html', [Path(filename).parent, Path(__file__).parent / 'templates']) | |
else: | |
# fall back to built-in simple template if no template was provided | |
templatefile = Path(__file__).parent / 'templates/simple.html' | |
templatefile = smart_file_resolve(f'{templatename}.html', [Path(filename).parent, STD_TEMPLATES]) | |
else: | |
# fall back to built-in simple template if no template was provided | |
templatefile = STD_TEMPLATES / 'simple.html' |
src/wireviz/wv_html.py
Outdated
with open_file_read(f'{filename}.svg') as file: | ||
svgdata = re.sub( | ||
'^<[?]xml [^?>]*[?]>[^<]*<!DOCTYPE [^>]*>', | ||
'<!-- XML and DOCTYPE declarations from SVG file removed -->', |
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.
Consider including the name of the embedded file in the leading comment for easier debugging:
'<!-- XML and DOCTYPE declarations from SVG file removed -->', | |
f'<!-- XML and DOCTYPE declarations from file "{file.name}" removed -->', |
src/wireviz/wv_html.py
Outdated
svgdata = re.sub( | ||
'^<[?]xml [^?>]*[?]>[^<]*<!DOCTYPE [^>]*>', | ||
'<!-- XML and DOCTYPE declarations from SVG file removed -->', | ||
file.read(), 1) |
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.
Consider also tagging the end of the embedded file:
file.read(), 1) | |
file.read(), 1) + f'<!-- End of file "{file.name}" -->' |
src/wireviz/wv_html.py
Outdated
# generate BOM contents | ||
bom_contents = [] | ||
for row in bom[1:]: | ||
row_html = ' <tr>\n' |
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.
Consider tagging each BOM row with the unique ID to allow cross referencing to each BOM entry:
row_html = ' <tr>\n' | |
row_html = f' <tr id="{row[0]}">\n' |
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.
Maybe it's even better with a prefix to reduce the probability of any future ID conflicts:
row_html = ' <tr>\n' | |
row_html = f' <tr id="bom-e{row[0]}">\n' |
src/wireviz/wv_html.py
Outdated
if isinstance(contents, (str, int, float)): | ||
replacements[f'<!-- %{item}% -->'] = html_line_breaks(str(contents)) |
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.
In case of a name collision between a metadata entry and any of the simple replacements above, the metadata entry will overwrite the simple replacement. Is this intentional? It enables overriding e.g. the generator version string.
for row in listy[1:]: | ||
file.write(' <tr>\n') | ||
for i, item in enumerate(row): | ||
item_str = item.replace('\u00b2', '²') |
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 cannot find this replacement in your new code. Is it obsolete and no longer needed, or handled in a different way?
New version of #74.
Reason for new PR:
feature/metadata
dev
.feature/metadata
(since it was no longer needed for # 214) without changing # 74's base first, cause it to auto-close, with no way to re-open