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

[tobiasfalk]

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😁🤦‍♂️

[kvid]

When v0.4.1 is released, then those fixes need to be merged with this PR into dev. It's a bit hard to guess how long that'll take.

Such fixes are normally a number of small changes only, but when the same part of the code is also refactored in this PR, then we need to rework each of those fixes.

This PR doesn't block all new features, but most HTML generating code is refactored here, so other major code changes in that part of the code based on the latest release (before this PR) will need rework before a merge-in after this PR.

[tobiasfalk]

Well, since it renames "Dataclasses.py" and "Harnesses.py" everything that needs to be implemented in either of those two has to be redone.
So is it a problem if I redo my changes from #369 and #379 based on this branch, and maybe even create a merge to this branch?
Mainly because I currently have time, and I do not know how much time I have when this is merged.
And for the future, when renaming of a file is done, this should be its own merge and this should be processed a bit faster so that other people do not have to reimplement their changes.
Ps. Sorry If I am a bit annoying.

[kvid]

If you have time for rework now, but maybe not later, then I suggest you consider creating two new PRs based on the dev branch, but with feature branches that you branch off from the current #251 branch. Then you have effectively included all current #251 changes in your new feature branches before reworking your old feature branches on top of each new feature branch (cherry-picking when you can and reimplementing when you must). If you also need any of the fixes to be released as v0.4.1, then you can cherry-pick that commit into your new feature branch before committing your code that depends on it.

This way it should be much easier to later rebase your new feature branches on top the new dev after the future #251 merge-in, even if the #251 feature branch has been rebased, force-pushed, and merged with v0.4.1 in the meantime. Such a rebase should remove all #251 changes and cherry-picked fixes from your feature branch unless a conflict needs resolving first. I recommend you keep both the old and new PRs as drafts until you prepare for actual merge-ins. Don't close your old PRs until successful merge-ins of the new ones.

Be aware that it's not safe to base your new PRs on the #251 feature branch, because when that branch is deleted after the #251 merge-in, all PRs based on a deleted branch will be automatically closed by github, and it's not easy to reopen such automatically closed PRs.

Ps. Sorry If I am a bit annoying.

I don't find you annoying! 😄 Maybe you mean "annoyed"?

[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).

[formatc1702]

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]

I just ran the examples and looked over it and found that with tutorial one the wires are not drawn correctly:

this does not happen when a color is defined:

Can easly be fixed with color = "#000000:#FFFFFF:#000000" in line 402 of wv_graphviz.py and

def gv_wire_cell(wire: Union[WireClass, ShieldClass], colspan: int) -> Td:
    if wire.color:
        color_list = ["#000000"] + wire.color.html_padded_list + ["#000000"]
    else:
        color_list = ["#000000","#FFFFFF","#000000"]

    wire_inner_rows = []
    for j, bgcolor in enumerate(color_list[::-1]):
        wire_inner_cell_attribs = {
            "bgcolor": bgcolor if bgcolor != "" and bgcolor != "#000000" else "#000000",

starting at line 366

[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]

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]

I just ran the examples and looked over it and found that with tutorial one the wires are not drawn correctly:

this does not happen when a color is defined:

Can easly be fixed with color = "#000000:#FFFFFF:#000000" in line 402 of wv_graphviz.py and

def gv_wire_cell(wire: Union[WireClass, ShieldClass], colspan: int) -> Td:
    if wire.color:
        color_list = ["#000000"] + wire.color.html_padded_list + ["#000000"]
    else:
        color_list = ["#000000","#FFFFFF","#000000"]

    wire_inner_rows = []
    for j, bgcolor in enumerate(color_list[::-1]):
        wire_inner_cell_attribs = {
            "bgcolor": bgcolor if bgcolor != "" and bgcolor != "#000000" else "#000000",

starting at line 366

[formatc1702]

Back in the day, drawing wires as thin when they don't have an explicit color assigned, was intentional, to distinguish an "uncolored" wire from a white or black one. For example, shields are also drawin in this style, since they don't have a color per se.

[tobiasfalk]

OK, didn't know that.

[tobiasfalk]

color_list = ["#000000","#FFFFFF","#000000"], see line 402

[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?