Large scale refactoring

[17o2]

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.

[17o2]

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

[17o2]

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.

[17o2]

• 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

[tobiasfalk]

Thx
Is this what you meant? https://github.com/tobiasfalk/WireViz/tree/Jumpers_reimp

[kvid]

@tobiasfalk wrote:

Thx Is this what you meant? https://github.com/tobiasfalk/WireViz/tree/Jumpers_reimp

That seems to be the result of creating Jumpers_reimp by branching it off at dev with a command like this: git branch --no-track Jumpers_reimp remotes/{official remote}/dev and then merged remotes/{official remote}/refactor/big-refactor into it.

In that case, I don't know if a later rebase of Jumpers_reimp on top of dev after #251 has been merged in, will remove the #251 changes. Personally, I usually avoid using merge unless I need to document joining two branches into one. When joining changes from two branches without such a need, I usually prefer cherry-picking and rebasing instead of merging, because the commit history becomes much easier to read, and git rebase automatically removes redundant commits.

I don't know the remote names of your local repo, but you can list them with git remote -v. Replace {official remote} in my commands with your remote name for our official public github repo, and {personal fork} with your remote name for your personal fork at github copied from our official public github repo.

These are the commands that represent what I suggested in my previous comment (new_feature1 can be whatever name you want):

git branch --no-track new_feature1 remotes/{official remote}/refactor/big-refactor
git push -u {personal fork} new_feature1

Update: I also strongly recommend not adding changes of generated files into feature branches. Following that advice will reduce the number of conflicts when rebasing (or merging).

[17o2]

I do not want to be to anything but is there a time frame of when this is merged, since this is kind of putting new features at hold😁🤦‍♂️

I fully agree and I feel bad that it has taken so long; it's a big change that I started on my own, but due to limited capacity I have not been able to complete it in a satisfactory way.

I am extremely thankful for all input recently, and the big help in developing, reviewing and integrating fixes. Currently, I can offer little more than some light guidance and clicking the final "merge" on PRs, or publishing a release to PyPI, but not much coding.

#365 is almost ready, and merging this refeactoring should be top prio afterwards. Again, I will appreciate the support here (especially from contributors more versed in resolving the ugly merge conflicts that may occur) as well as in the subsequent bugfixing and integrating all fixes made to the old codebase in the meantime (from v0.4 to v0.4.1).

So, big ❤️ to the active contributor community.

@kvid if you can, please have a look at our private chat. Thx!

[tobiasfalk]

#365 is merged, I would suggest we now merge this one and create a Task List to make working on this easier.

[kvid]

#365 is merged, I would suggest we now merge this one and create a Task List to make working on this easier.

I totally agree, and I've started some testing, but I'm quite busy at my paid work this month. Up to now, I've found a few bugs, but as they also seem to be present in v0.4.1, I plan to describe them in a new issue to be fixed later.

If you have the time and the enery, you are welcome to do your own review in parallel, maybe describe what you tested, and suggest alternatives.

My idea is to bring in the v0.4.1 changes stepwise into dev after this PR because some of the v0.4.1 changes need to be reimplemented due to the refactored code. What do you think?

[tobiasfalk]

If you have the time and the enery, you are welcome to do your own review in parallel, maybe describe what you tested, and suggest alternatives.

I will look in to it, but I do not relay have much experience in code testing and reviewing.

My idea is to bring in the v0.4.1 changes stepwise into dev after this PR because some of the v0.4.1 changes need to be reimplemented due to the refactored code. What do you think?

Sounds good, as mentioned, I would start with a simple List of what needs to be transplanted/reimplemented.

Two of the big points that I would put on this Task List, would be #406 and #387, since they would make communication with the users easier and take the work off the developers, yes even though they practically have nothing to do with this PR.

[tobiasfalk]

GitHub has something called Projects (here the FreeCAD Git for example: https://github.com/orgs/FreeCAD/projects) this would be a good solution to group the relevant Issues and so.

[tobiasfalk]

opend a review for those two

[tobiasfalk]

Also, something with the Line Thickness of the wire block is not right:

this can also be solved rather simply with seting tbl = Table(rows, border=1, cellborder=0, cellspacing=0) at 362 wv_graphviz.py

[tobiasfalk]

@kvid and @formatc1702, how does it stand with merging this and creating a todo list of things from 0.4.1 that need to be reimplemented?

[ssweber]

Has there been any discussion to unblock this? The WIP shorts/jumpers and wire-banding would really make WireViz a great output format to document control cabinets.

[17o2]

Unfortunately, all WireViz development* is on hiatus from my side due to personal reasons. @kvid is working on this as time allows, and has merging rights. I'm sorry I don't have better news.

* I can still take care of PyPI publishing.

[kvid]

The latest issue I've worked on, is to find the missing steps to fulfill #408 (see my post-merge-in comments there) - that is currently blocking all PRs - including this big one. However, unforeseen circumstances have forced me to put most of my effort on hold for a period. During the last weeks, I've only been able to answer a few issue comments.

Update: I just created issue #441 to describe the problem properly.

[17o2]

Closing PR after using #447 to simplify the merge process.