Skip to content
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

Merged
merged 59 commits into from
Jun 7, 2023

Conversation

formatc1702
Copy link
Collaborator

@formatc1702 formatc1702 commented Aug 25, 2021

New version of #74.

Reason for new PR:

  • 74 was based on the original branch for Add basic options and metadata #214, called feature/metadata
  • # 214 was then squash-merged onto dev.
  • Deleting 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

@formatc1702 formatc1702 marked this pull request as ready for review August 26, 2021 17:18
@formatc1702
Copy link
Collaborator Author

formatc1702 commented Aug 26, 2021

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:

  • Optimization of the HTML templates themselves
  • Dealing with resolving template file paths specified in the prepend YAML file.
    • I would like to fix this together with the related issue from the comment in #189 afterwards
    • The newly added smart_file_resolve() may help fix both issues
  • Parsing any color/font specs from the options section of the YAML and applying them to the HTML
  • I have purposely only edited demo02.yml to include metadata and template specs, but not demo01.yml, so as to keep 01 as simple as possible.

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.

  • 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% -->)

src/wireviz/wv_helper.py Outdated Show resolved Hide resolved
src/wireviz/wv_html.py Outdated Show resolved Hide resolved
src/wireviz/templates/din-6771.html Outdated Show resolved Hide resolved
src/wireviz/templates/simple.html Outdated Show resolved Hide resolved
@formatc1702
Copy link
Collaborator Author

formatc1702 commented Sep 27, 2021

  • This preliminary review is only based on inspection of changes, not testing.

Thanks, I will incorporate some of your changes soon and wait for some testing.

👍

  • Have you considered supporting an optional default value of each keyword in templates? (e.g. something like this: <!-- %bgcolor%white% -->)

Sounds like a nice follow-up PR.

src/wireviz/wv_html.py Outdated Show resolved Hide resolved
src/wireviz/wv_html.py Outdated Show resolved Hide resolved
@formatc1702 formatc1702 requested a review from kvid October 2, 2021 16:09
src/wireviz/wv_html.py Outdated Show resolved Hide resolved
src/wireviz/wv_html.py Outdated Show resolved Hide resolved
src/wireviz/wv_html.py Outdated Show resolved Hide resolved
src/wireviz/wv_helper.py Outdated Show resolved Hide resolved
src/wireviz/wv_html.py Outdated Show resolved Hide resolved
@formatc1702 formatc1702 changed the base branch from dev to feature/mate+autogenerate October 15, 2021 12:20
- 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
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 like this concept of templates and the generic replacement rules. However, I have included a few more suggestions.

Comment on lines +151 to +152
for possible_path in possible_paths:
resolved_path = (possible_path / filename).resolve()
Copy link
Collaborator

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:

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

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?

Copy link
Collaborator

@kvid kvid Jan 8, 2022

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?

Comment on lines 153 to 157
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]))
Copy link
Collaborator

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 testing is_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
Suggested change
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]))

Comment on lines 18 to 21
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'
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 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:

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

with open_file_read(f'{filename}.svg') as file:
svgdata = re.sub(
'^<[?]xml [^?>]*[?]>[^<]*<!DOCTYPE [^>]*>',
'<!-- XML and DOCTYPE declarations from SVG file removed -->',
Copy link
Collaborator

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:

Suggested change
'<!-- XML and DOCTYPE declarations from SVG file removed -->',
f'<!-- XML and DOCTYPE declarations from file "{file.name}" removed -->',

svgdata = re.sub(
'^<[?]xml [^?>]*[?]>[^<]*<!DOCTYPE [^>]*>',
'<!-- XML and DOCTYPE declarations from SVG file removed -->',
file.read(), 1)
Copy link
Collaborator

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:

Suggested change
file.read(), 1)
file.read(), 1) + f'<!-- End of file "{file.name}" -->'

# generate BOM contents
bom_contents = []
for row in bom[1:]:
row_html = ' <tr>\n'
Copy link
Collaborator

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:

Suggested change
row_html = ' <tr>\n'
row_html = f' <tr id="{row[0]}">\n'

Copy link
Collaborator

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:

Suggested change
row_html = ' <tr>\n'
row_html = f' <tr id="bom-e{row[0]}">\n'

Comment on lines 71 to 72
if isinstance(contents, (str, int, float)):
replacements[f'<!-- %{item}% -->'] = html_line_breaks(str(contents))
Copy link
Collaborator

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', '&sup2;')
Copy link
Collaborator

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?

@formatc1702 formatc1702 mentioned this pull request Aug 5, 2022
5 tasks
Base automatically changed from feature/mate+autogenerate to dev June 7, 2023 17:11
@formatc1702 formatc1702 merged commit e31ed72 into dev Jun 7, 2023
@formatc1702 formatc1702 deleted the feature/technical-drw branch June 7, 2023 18:01
@kvid kvid restored the feature/technical-drw branch September 1, 2023 18:45
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