Feature/gv html refactor

[formatc1702]

Fixes #66.

Adds a color property to cables, to match the behavior of connectors, for consistency.

This might be useful, for example, for highlighting cables that remain live after an emergency stop or mains disconnection, that need to be orange-coloured according to certain standards (Source in German).

[formatc1702]

This is not meant to be the definitive refactor, since there will be a lot of changes introduced in v0.3 onward, but it cleans up quite a bit and should be enough for now.

[formatc1702]

Good calls, all addressed in the latest commit. Thanks!
Let me know if there's anything else, or give the green light otherwise :)

[kvid]

I also suggest to remove line breaks from manufacturer info written to the BOM. I was not able to comment directly on the lines you didn't change now, but here is the full diff:

---------------------------- src/wireviz/Harness.py ----------------------------
index 69e0443..4c93895 100644
@@ -197,8 +197,9 @@ class Harness:
                     wireidentification = []
                     if isinstance(cable.pn, list):
                         wireidentification.append(f'P/N: {cable.pn[i - 1]}')
-                    manufacturer_info = manufacturer_info_field(cable.manufacturer[i - 1] if isinstance(cable.manufacturer, list) else None,
-                                                                      cable.mpn[i - 1] if isinstance(cable.mpn, list) else None)
+                    manufacturer_info = manufacturer_info_field(
+                        cable.manufacturer[i - 1] if isinstance(cable.manufacturer, list) else None,
+                        cable.mpn[i - 1] if isinstance(cable.mpn, list) else None)
                     if manufacturer_info:
                         wireidentification.append(html_line_breaks(manufacturer_info))
                     # print parameters into a table row under the wire
@@ -335,7 +336,7 @@ class Harness:
             conn_color = f', {shared.color}' if shared.color else ''
             name = f'Connector{conn_type}{conn_subtype}{conn_pincount}{conn_color}'
             item = {'item': name, 'qty': len(designators), 'unit': '', 'designators': designators if shared.show_name else '',
-                    'manufacturer': shared.manufacturer, 'mpn': shared.mpn, 'pn': shared.pn}
+                    'manufacturer': remove_line_breaks(shared.manufacturer), 'mpn': remove_line_breaks(shared.mpn), 'pn': shared.pn}
             bom_connectors.append(item)
             bom_connectors = sorted(bom_connectors, key=lambda k: k['item'])  # https://stackoverflow.com/a/73050
         bom.extend(bom_connectors)
@@ -354,7 +355,7 @@ class Harness:
             shield_name = ' shielded' if shared.shield else ''
             name = f'Cable{cable_type}, {shared.wirecount}{gauge_name}{shield_name}'
             item = {'item': name, 'qty': round(total_length, 3), 'unit': 'm', 'designators': designators,
-                    'manufacturer': shared.manufacturer, 'mpn': shared.mpn, 'pn': shared.pn}
+                    'manufacturer': remove_line_breaks(shared.manufacturer), 'mpn': remove_line_breaks(shared.mpn), 'pn': shared.pn}
             bom_cables.append(item)
         # bundles (ignores wirecount)
         wirelist = []
@@ -364,8 +365,8 @@ class Harness:
                 # add each wire from each bundle to the wirelist
                 for index, color in enumerate(bundle.colors, 0):
                     wirelist.append({'type': bundle.type, 'gauge': bundle.gauge, 'gauge_unit': bundle.gauge_unit, 'length': bundle.length, 'color': color, 'designator': bundle.name,
-                                     'manufacturer': index_if_list(bundle.manufacturer, index),
-                                     'mpn': index_if_list(bundle.mpn, index),
+                                     'manufacturer': remove_line_breaks(index_if_list(bundle.manufacturer, index)),
+                                     'mpn': remove_line_breaks(index_if_list(bundle.mpn, index)),
                                      'pn': index_if_list(bundle.pn, index)})
         # join similar wires from all the bundles to a single BOM item
         wire_group = lambda w: (w.get('type', None), w['gauge'], w['gauge_unit'], w['color'], w['manufacturer'], w['mpn'], w['pn'])

[formatc1702]

Will include both your suggestions. Maybe the new CSV library coming in #123 will handle line breaks in a better way, too.

One more thing I am thinking about is replacing all

html = f'{html}<some code>`

with

html = []
html.append('<some code>')
...
html='\n'.join(html)

which would make running diff on the .gv files much easier since the HTML is split into lines, and is perhaps more elegant than just doing

html = f'{html}<some code>\n`

for every line. Will play with this when I have more time.

[kvid]

Will include both your suggestions.

Thank's.

Maybe the new CSV library coming in #123 will handle line breaks in a better way, too.

Testing is needed, I guess...

One more thing I am thinking about is replacing all

html = f'{html}<some code>`

with

html = []
html.append('<some code>')
...
html='\n'.join(html)

which would make running diff on the .gv files much easier since the HTML is split into lines, and is perhaps more elegant than just doing

html = f'{html}<some code>\n`

for every line. Will play with this when I have more time.

I really like your idea! 👍 You are also getting closer to something similar to nested_html_table() it seems.

[formatc1702]

Implementation is complete, I'm quite pleased with the result (especially the much more readable .gv files) and I'm ready to merge once @kvid agrees :)

[kvid]

• I really like this new output with the HTML split into many '\n' separated rows of code that simplify code inspection during development and reduce the diffs! 😃 I would have preferred moving the label attributes containing HTML to the end of each node declaration to avoid "hiding" the remaining node attributes located after all HTML lines, but unfortunately, it seems the dot.node() function have hardcoded the label to be first attribute.

• I'm not quite sure if I have done this review correctly. 😟 Some of the code fragments I have commented below, are marked as Outdated, and I don't know why.

• Several of my comments are just different instances of the same issue. Each issue is explained at least once, and the other issue instances might have a simplified comment only, but my comments below are unfortunately not shown in the same order as when I wrote the comments, and therefore the explaining comment might not be the first one in all cases.

• You might not share my opinion about all these issues, and I respect that. These two issues, in particular, I'm not completely convinced about (maybe it's better to just keep your code version in these cases?):

  • Replacing consecutive append() calls with a single extend() call? (Mixing append() and extend() a lot might be bad for consistency and readability.)
  • Avoid reusing the variable when converting to another type? (Maybe OK as long as there is no doubt about what it contains.)

• When there is a good reason for appending to a list that is just initialized to an empty list, then I also suggest adding a small comment that explains this reason, e.g. something like this:

    html = [] # Using append() to assign the first row below for consistency with the other rows
    html.append('<table...>')

• I have used the term row in several of my suggestions (e.g. above) where I talk about '\n' separated rows of HTML code. Do you think this term might be misunderstood as HTML table rows?

• I'm sorry for creating all the noise with a high number of comments about repeated issues that I'm not fully convinced about. 😢 However, there are also a few other minor issues, and I hope they don't drown in the pool of repeated issues.

• I wonder why you have chosen to first generate an outer table containing some placeholder comments, then generate a new inner table, and then replacing any placeholder in the outer table with the inner table? Isn't it possible (and more readable code) to first building the inner table, and then inserting that directly (instead of a placeholder) when building the outer table? When you need to replace some part of the tag that nested_html_table() inserts just before the placeholder, then I understand a replace() must be done afterwords, but that's only needed for the <!-- colorbar --> placeholders. It's not a major issue - I'm just curious.

[kvid]

Consider html = nested_html_table(rows) instead, or is there a good reason behind extending an empty list?

[formatc1702]

In this case I find it more understandable to start with an empty list (line 91) and then populate it slowly... a matter of style, nothing else.

[kvid]

Consider pinhtml = ['<table border="0" cellspacing="0" cellpadding="3" cellborder="1">'] instead, or is there a good reason behind appending to an empty list?

[kvid]

Consider htmlstr = '\n'.join(html) to avoid reusing the list variable as a string variable.

[formatc1702]

In this case I will leave it as is, since the join can't be built into the f-String easily... the variable is never used again after the following line, so I don't see an issue with reusing it.

[kvid]

Consider pinhtmlstr = '\n'.join(pinhtml) to avoid reusing the same variable with another type. There are different opinions about this subject, though. Feel free to ignore my comments about this. There are several instances of this issue in the code, and I assume you either keep them all as is, or change them all.

[formatc1702]

I don't think it's terrible to reuse the name in this instance, but I have come up with a compromise that doesn't require pinhtmlstr either.

html = [row.replace('<!-- connector table -->', '\n'.join(pinhtml)) for row in html]

[kvid]

@formatc1702 When trying to rebase my PR #137 on top of this branch, I discovered an issue that I didn't see during my last review:

• Your refactoring has changed the rendering style of cable.notes: Now, there is a full border around notes (not dashed for bundles), and no spacer below notes.
• Are these rendering differences intentional?
• My PR Add optional image and caption attributes to Connector and Cable/Bundle nodes #137 copied the notes style currently in dev, and I have to know if your change of style also should apply to my PR.
  

[formatc1702]

@kvid, I have built in some of your suggested changes. I've left the .extend() instances since I prefer explictly creating an empty list first and then filling it... just a matter of taste 🤷 I've marked duplicates of your comments as resolved, as well as any suggestions I agree with and have used as-is.

Regarding borders around bundle notes: It's a side-effect of refactoring but I would argue it is more consisten this way... Otherwise the notes just float around below the wires and their manufacturer info.
If you are fine with the latest changes, give me the green light and I'll merge :)

[kvid]

@kvid, I have built in some of your suggested changes. I've left the .extend() instances since I prefer explictly creating an empty list first and then filling it... just a matter of taste 🤷 I've marked duplicates of your comments as resolved, as well as any suggestions I agree with and have used as-is.

That's perfectly fine with me. As I wrote in the main comment of #136 (review), I was not fully convinced about the single extend() and the reuse variable issues. In fact, I kind of changed my mind after submitting my review when I took a new look at the whole picture.

Regarding borders around bundle notes: It's a side-effect of refactoring but I would argue it is more consisten this way... Otherwise the notes just float around below the wires and their manufacturer info.

I don't disagree with you. I just wanted to know it was intentional, and not something you would like to reverse very soon. 😃

If you are fine with the latest changes, give me the green light and I'll merge :)

I only have my phone here now, and are unable to test your new commit until I'm back to my PC probably later today.

[kvid]

Thank you for reading my review and accepting a selection of my suggestions. I hope you also read my main review comment #136 (review), and not only the code fragment comments.

@formatc1702 wrote:

I don't think it's terrible to reuse the name in this instance, but I have come up with a compromise that doesn't require pinhtmlstr either.

html = [row.replace('<!-- connector table -->', '\n'.join(pinhtml)) for row in html]

As long as you are not afraid of a potential increase of running-time (I have not done measurements) when doing the join inside the loop for each row of the connector HTML and similar for the cable HTML, then go ahead merging.

The HTML output looks quite good. Do you have a set of rules or recommendations about where to insert newlines, i.e. what HTML tags can be on the same row, and when should they be separated? Where should such a description be stored and updated to improve consistency when new HTML entries are introduced in the future? Maybe close to the nested_html_table() implementation?

Have you considered a single space indentation for each sub-level of HTML tag structure? The up-side is HTML readability, but the down-side is an increased diff when moving sections to a different sub-level.

[formatc1702]

I hope you also read my main review comment #136 (review), and not only the code fragment comments.

Thanks for bringing the comment to my attention. I read it when you first posted it, but I must have missed some of the later edits. I will address some open points below; most of them have already been addressed elsewhere.

As long as you are not afraid of a potential increase of running-time

I'm not worried about runtime for WireViz at this moment, and there are probably bigger issues in this respect anyway. Comparing the performance on this issue is probably more of academit interest ;-)

Do you have a set of rules or recommendations about where to insert newlines, i.e. what HTML tags can be on the same row, and when should they be separated?

No hard rules right now, and it's not terribly important. Cells are usually in one row (<td>...</td>). Multiple short opening tags in sequence tend to go together (<tr><td> and the respective closing tags for the outermost table, for example). Row breaks (</tr><tr>) could go in one line but are usually split because they come from iterations of a loop.

Have you considered a single space indentation for each sub-level of HTML tag structure?

I thought about it but decided against it; the gains are marginal, the readability of the Python code would suffer; plus it's easy to pass the HTML through some auto-formatter afterwards if one really needs to analyze the structure at some point.
Edit: thinking about it some more, it might not be such a bad idea, I'll try it out and push a commit.

---

About your comment

unfortunately, it seems the dot.node() function have hardcoded the label to be first attribute.

Annoying, yes. Oh well...

I'm not quite sure if I have done this review correctly. 😟 Some of the code fragments I have commented below, are marked as Outdated, and I don't know why.

Not sure when you added this sentence... if I push new commits that address your issues, they become outdated since the commit they refer to is no longer the current one. At least that is my theory. So it's probably a good sign in this case.

When there is a good reason for appending to a list that is just initialized to an empty list, then I also suggest adding a small comment that explains this reason

I think the comment adds more noise than the extra html = [] line itself, so I don't like the idea too much, TBH.

I wonder why you have chosen to first generate an outer table containing some placeholder comments [...]

It's a bit of a development artifact, yes. Part of me agrees with you that inserting the content directly makes more sense. Part of me likes defining the broad structure first, and then filling in the details, and "hacking" the existing code to change the bgcolor of the color bar... I'll think about this and might restructure it to be more... conventional ;-)

Please let me know if there are any other open points I should address!

[kvid]

I'm not worried about runtime for WireViz at this moment, and there are probably bigger issues in this respect anyway. Comparing the performance on this issue is probably more of academit interest ;-)

I agree. It was more like a joke. 😄

Do you have a set of rules or recommendations about where to insert newlines, i.e. what HTML tags can be on the same row, and when should they be separated?

No hard rules right now, and it's not terribly important. Cells are usually in one row (<td>...</td>). Multiple short opening tags in sequence tend to go together (<tr><td> and the respective closing tags for the outermost table, for example). Row breaks (</tr><tr>) could go in one line but are usually split because they come from iterations of a loop.

Nice. Should we but these words into a comment in the code as soft recommendations?

Maybe into wv_helper.py, or into a new wv_html.py and move the increasing number (at least in my PR #137) of html related helper functions there too?

Have you considered a single space indentation for each sub-level of HTML tag structure?

I thought about it but decided against it; the gains are marginal, the readability of the Python code would suffer; plus it's easy to pass the HTML through some auto-formatter afterwards if one really needs to analyze the structure at some point.
Edit: thinking about it some more, it might not be such a bad idea, I'll try it out and push a commit.

It is not a major issue to me, especially if the Python code gets more complicated and less readable. I will wait and see what you suggest.

I think the comment adds more noise than the extra html = [] line itself, so I don't like the idea too much, TBH.

I see your point, and have changed my mind. I now agree with you.

I wonder why you have chosen to first generate an outer table containing some placeholder comments [...]

It's a bit of a development artifact, yes. Part of me agrees with you that inserting the content directly makes more sense. Part of me likes defining the broad structure first, and then filling in the details, and "hacking" the existing code to change the bgcolor of the color bar... I'll think about this and might restructure it to be more... conventional ;-)

See commit 9c933d1 in PR #137 where I changed the colorbar code to something similar to my newest image code. That PR is now based on this PR and using your new refactoring, by the way.

Please let me know if there are any other open points I should address!

None that I can think of right now...

[formatc1702]

Do you have a set of rules or recommendations about where to insert newlines, i.e. what HTML tags can be on the same row, and when should they be separated?

No hard rules right now, and it's not terribly important. Cells are usually in one row (<td>...</td>). Multiple short opening tags in sequence tend to go together (<tr><td> and the respective closing tags for the outermost table, for example). Row breaks (</tr><tr>) could go in one line but are usually split because they come from iterations of a loop.

Nice. Should we but these words into a comment in the code as soft recommendations?

Maybe into wv_helper.py, or into a new wv_html.py and move the increasing number (at least in my PR #137) of html related helper functions there too?

A new module for the HTML stuff sounds good, as a separate issue/PR!

Have you considered a single space indentation for each sub-level of HTML tag structure?

I thought about it but decided against it; the gains are marginal, the readability of the Python code would suffer; plus it's easy to pass the HTML through some auto-formatter afterwards if one really needs to analyze the structure at some point.
Edit: thinking about it some more, it might not be such a bad idea, I'll try it out and push a commit.

It is not a major issue to me, especially if the Python code gets more complicated and less readable. I will wait and see what you suggest.

It was less painful than anticipated, and I do like the output! Have a look at the latest commit. This will hopefully be the last change before the merge.

[kvid]

What is the motivation for this merge? I can't see any real change of code. What is then gained, besides more complex commit history?

[formatc1702]

What is the motivation for this merge? I can't see any real change of code. What is then gained, besides more complex commit history?

You mean 97a9de4? I just discovered it too. I have absolutely no idea where it came from, or why it happened... especially since no code changes 😕

This will hopefully be the last change before the merge.

I was just referring to merging this feature into dev BTW, nothing weird ;-)

[formatc1702]

Should be fixed now. Very strange.

On a separate note:
I'd actually like to rebase this feature onto dev and squash some commits but I'm worried it will mess with your commit history on #137 since it branches off the current feature branch and it might lose its parent commit. Any ideas?

[kvid]

Should be fixed now. Very strange.

Thank's for the fix.

On a separate note:
I'd actually like to rebase this feature onto dev and squash some commits but I'm worried it will mess with your commit history on #137 since it branches off the current feature branch and it might lose its parent commit. Any ideas?

You can go ahead rebasing. My repo will still contain copies of your current commits, and I can rebase my branch on top of your updated branch afterwards.

[formatc1702]

All good regarding the HTML whitespace feature then?

[kvid]

I agree. The output look good, and don't think the Python code readability was harmed.