Large scale refactoring

[formatc1702]

This is a project wide refactoring effort.
I have decided to address it as one large chunk because the fundamental structure of the code needs to be rebuilt at once for this to work.

Goals:

• Shorter, more atomic functions
• Shallower indentation
• More object-oriented (HTML tags, pins, wires, mates, colors, ...)
• Less hand-crafted HTML, fewer hacky tricks
• Less convoluted code
• Less code repetition

Issues being addressed/solved:

• Implement the ideas initially tried in BOM generation refactoring #249
• [bug] Bundles to have consistent part number fields #183
• [feature] improved multilingual color input #135
• Improving additional component display, BOM item numbers #224
• Better additional component BOM table #226

A better description will follow once the code is presentable.
I am sharing the work in progress for transparency's sake.

[amotl]

Dear Daniel,

I just wanted to drop you a line that I appreciate your efforts on this task very much.

With kind regards,
Andreas.

[formatc1702]

I have resumed work on the refactoring after a longer period of inactivity. and I would like to propose a course of action to get the bulk of this code integrated into dev, and return to incremental improvements through smaller PRs.

• @kvid if you have time, please check that there are no glaring errors when building the example/tutorial files, regarding the graphical output and the BOM.
• Even if the examples build without errors, the code will probably be rough around the edges. Nevertheless, I would then like to merge this PR into dev to make it "visible" to the larger group of contributors. And, importantly, this will take a big mental load off of me to know we are back to working in manageable chunks 😺
• I realize that there are some open discussions and suggestions in the PRs this one is built on top of; i.e. the ones with the in latest tag. I do not want these discussions/suggestions in these PRs to be lost, therefore the plan is:
  • Leave those PRs open for now if they include important unresolved comments.
  • Address the suggestions as new commits on top of this PR, to avoid nasty rebasing troubles.
  • Close the old PRs once I feel that everything is addressed.
    This concerns Improve output of diagram + BOM as technical drawing #239, Replace os.path with pathlib.Path #246, and to a lesser extent Implement mates between connectors, better autogeneration #186, Embed images in SVG output #189. There are no comments on Clean up and improve CLI implementation #244, Apply isort and black to source code #248, Improve wireviz.parse() #250; any improvements on these topics should come as new issues/PRs.
• The groundwork has been laid for some other open feature request (such as BOM stuff from Improving additional component display, BOM item numbers #224 + Better additional component BOM table #226).. full implementation will follow in new PRs.

Thank you for your patience and understanding.

[kvid]

@formatc1702 wrote:

I have resumed work on the refactoring after a longer period of inactivity. and I would like to propose a course of action to get the bulk of this code integrated into dev, and return to incremental improvements through smaller PRs.

• @kvid if you have time, please check that there are no glaring errors when building the example/tutorial files, regarding the graphical output and the BOM.

I'd be happy to do that, but the installation failes as described in #287.

• Even if the examples build without errors, the code will probably be rough around the edges. Nevertheless, I would then like to merge this PR into dev to make it "visible" to the larger group of contributors. And, importantly, this will take a big mental load off of me to know we are back to working in manageable chunks 😺

I agree this sounds like a wise decision.

• I realize that there are some open discussions and suggestions in the PRs this one is built on top of; i.e. the ones with the in latest tag. I do not want these discussions/suggestions in these PRs to be lost, therefore the plan is:

  • Leave those PRs open for now if they include important unresolved comments.
  • Address the suggestions as new commits on top of this PR, to avoid nasty rebasing troubles.
  • Close the old PRs once I feel that everything is addressed.
    This concerns Improve output of diagram + BOM as technical drawing #239, Replace os.path with pathlib.Path #246, and to a lesser extent Implement mates between connectors, better autogeneration #186, Embed images in SVG output #189. There are no comments on Clean up and improve CLI implementation #244, Apply isort and black to source code #248, Improve wireviz.parse() #250; any improvements on these topics should come as new issues/PRs.

OK

[formatc1702]

One thing I still want to add is updated documentation / changelog. And I might rearrange some code blocks to make more sense.. but no further actual development.

[kvid]

I've not yet had time to test all new code, but I list here a few issues I've noticed so far:

• The code building examples still use the .bom.tsv extension. I suggest we use a common tuple of supported extensions that also can be available for applications using WireViz as a library.
• I don't like that I now must run wv_cli.py from the command line instead of the much more intuitive wireviz.py.
• It seems the GV HTML are generated differently now, and I can see some more attributes are generated. We should perhaps create a test case with bgcolor declared in all possible ways to verify that the intended logic with default values inherited from the more general level still holds, etc.

[formatc1702]

• I don't like that I now must run wv_cli.py from the command line instead of the much more intuitive wireviz.py.

The idea is that wireviz.py contains the core parsing and output functions (both file and return type), and it's what you would import when using WireViz as a module.
wv_cli.py splits off the CLI handling. In practice, users would not call python3 wv_cli.py, but simply invoke wireviz directly using the entry point after installing with pip. This is also how the usage is described now in the Readme.

I agree that testing bgcolor etc. is a good idea, since it's not in the current examples and I haven't used it much myself.

[amotl]

The idea is that wireviz.py contains the core parsing and output functions (both file and return type), and it's what you would import when using WireViz as a module. On the other hand, wv_cli.py splits off the CLI handling, and gets materialized as wireviz program using a console script entry point, when installing the package with pip.

I like it!

@kvid: I don't know if it's in the documentation yet, but the recommended way to "install" that entrypoint, while in development/sandbox mode, is to invoke pip install --editable=. in the repository root directory, where your setup.py, setup.cfg, or pyproject.toml resides.

This will run the installation process in development mode (what was python setup.py develop up until recently) and will link the defined entrypoints with your local working tree, so you can run wireviz and invoke the code you are currently editing. It is always recommended to do all of that within a Python virtualenv, but I am sure I am telling no news here.

Edit: I see that [1] may be a bit thin. Let me replicate a full sandbox installation example here if you don't mind:

git clone https://github.com/formatc1702/WireViz --branch=refactor/big-refactor
cd WireViz
python3 -m venv .venv
source .venv/bin/activate
pip install --editable=.
wireviz --version

WireViz 0.4-dev

[1] https://github.com/formatc1702/WireViz/blob/master/docs/README.md#installing-the-development-version

[kvid]

I pushed yesterday a couple of commits I'm using to:

• Correct .tsv filename references to match the rename earlier in this PR. I reported parts of this earlier in Large scale refactoring #251 (comment).
• Include extra YAML input files when building examples - to detect errors and for verifying the code after rebasing.

I'm currently investigating some strange generated HTML.

[formatc1702]

I think the biggest issues that should probably remain inside the scope of this PR (based on a project-wide search for TODO) are:

• Part number strings in gv_additional_component_table(), create_graph()
• Handling additional components in _add_to_internal_bom()(see TODO there, I can't recall why bundles require a special treatment)
• BOM hash for bundles in bom_hash()
• The above-mentioned packaging flaw

Overall I agree with @amotl 's and @kvid 's assessments. All other TODOs can become separate PRs

[kvid]

It's not clear to me how to use amount and sum_amounts_in_bom. They are not mentioned in syntax.md and not used in any example/tutorial/test harness. I hope @formatc1702 can provide some examples.

If these and qty are not supported in the Connector class, why are they all defined in this common superclass?

[formatc1702]

Sorry for the confusion. It stems from a wish to be able to specify how to handle lengths in the BOM.

Example:
A harness contains two identical cables of 1m each, another cable of same type but 5m, and two bundles of four identical wires of 1m each.

1. sum_amounts_in_bom = True would show the BOM requiring 7m of cable + 8m of wire
   (that is the current behavior)
2. sum_amounts_in_bom = False would show the BOM requiring 2 pcs. of cable (1m) + 1 pcs. of cable (5m) + 8 pcs. of wire (1m)

In a workflow where the person asssembling the harness is expecting to cut the cables themselves, option 1 is fine. If they are expecting to receive all cables/wires cut to length, option 2 is the way to go.

Another typical example is heatshrink: If there is a connector that requires 10mm of heatshrink on each of its 15 pins, and 30mm of the same heatshrink on the shield, the BOM could show either

1. 180mm of heatshrink, or
2. 15 pcs. of 10mm heatshrink + 1 pc. of 30mm heatshrink, which is much more understandable in context.

This is where the amount property comes in. The heatshrink would be defined as an additional component with qty: 15 and amount: 10 mm, and a separate additional component would have qty: 1 and amount: 30 mm.

If these and qty are not supported in the Connector class, why are they all defined in this common superclass?

Because they are also intended for use with AdditionalComponent, as in the heatshrink example.