Implement mates between connectors, better autogeneration

[formatc1702]

This is a sandbox to try an implementation of #134 and #184 (with possible effects for #142 and #185).
Since both involve changing the WireViz syntax, I figured it might make sense to tackle both at the same time, to make sure there are no conflicts between the two syntax additions.

• Discussion regarding the new syntax for mating connectors happens in [feature] Connector-Connector & Connector-Ferrule mating #134.
• Discussion regarding the new syntax for auto-generated components happens in [feature] Improved auto-generated connectors and cables #184.

This PR will try to reflect the results in those discussions.

The implementation will work towards being able to parse the included test/test1.yml and produce the expected result. This test file will try to reflect and test any related new features suggested in the discussions.

No detailed code review is necessary at the moment, consider this a work in progress;

[formatc1702]

The current state is able to parse all current examples/tutorials/demos (only minor changes to the source file necessary), as well as all test files, inspired by the discussions in the issues mentioned above. See tests/ directory for sources.

Main open TODO is re-implementing the check for alternating connectors and cables inside each connection set.

See renderings

Test 1

Test 2

Test 2

Test 3

Test 4

Test 9

[formatc1702]

Even though I have not worked on this feature for a while, I have been using it in practice and it has proven quite robust.

I would like to request a review of the code, and I am open for ideas on how to solve the open TODO...

@kvid how do you feel about this?

[formatc1702]

• The branch has been rebased onto dev
  (using squash to avoid needing to solve merge conflicts on intermediate commits;
  a backup branch exists for now to preserve the original history)
• All checks are passing and no merge conflicts await
• Output for all examples and tutorials looks good
• There are some minor TODOs which I would like to address in separate PRs to avoid delays (the branch has been essentially unchanged for almost a year)
• The code has seen months of real life use (by me) without problems
• It works as well as / better than the old code

I realize this is a major refactoring of the WireViz core. Nevertheless, it would be great to merge this to add some valuable new functionality to the project.

I am toying with the idea of trying out the "show" type PR merge as proposed by Martin Fowler to speed things up. The linked article is based on having a proper CI system; the bullet points above support the claim that the PR is mature. @kvid please share your thoughts on this :) thanks

[awb1015]

Just an anecdotal comment, I had been using this branch (prior to rebase) almost exclusively for several months now without issue. Have now updated my local to be after the rebase and will comment if I have any issues.

A merge into dev would be greatly appreciated assuming everything is ok.

[formatc1702]

Thanks @awb1015 for the feedback!

Usually I've waited for @kvid's approval before merging into dev, since he's quite good at catching edge cases, improving the code quality and providing generally valuable feedback. But as you can see by my previous comment, this might be a good PR to try out a new merging strategy, with post-hoc review and improvement.
I realize merging without a formal review would be an unilateral decision by me, going against the (spoken and unspoken) flow so far, and might be seen as hypocritical since kvid and others have had to wait for months for me to react to PRs and issues in the past, so I have been shy about this approach until now.

[awb1015]

@formatc1702 No problem, definitely not trying to rush the process or skip any amount of review.

I have noticed over the last few hours, since moving over to the post-rebase version of the branch, that mating arrows between pairs of mating connectors aren't shown properly.

Prior to rebase I hadn't had any issues but now in the output .png I've noticed that the arrows between equivalent pins are not fully aligned to the row of the connector. Or in a few cases they're coming out from the corner of a connector and going to its mate's corner as well.

[formatc1702]

Prior to rebase I hadn't had any issues but now in the output .png I've noticed that the arrows between equivalent pins are not fully aligned to the row of the connector. Or in a few cases their coming out from the corner of a connector and going to its mate's corner as well.

Could you provide some .png and the matching .yml and intermediate.gv files where you are having issues? Ideally both pre- and post-rebase, to make it easier. Using Gist might be easier to share the data than to paste it here directly.

[awb1015]

I've put together a sample to show the issue I'm attempting to describe.

This https://gist.github.com/awb1015/8019172cc474c3e0f98999f74a2fa387 contains the YAML file as well as the intermediate files generated both before and after a rebase.

I've also attached the respective .png files from before and after the rebase here. These show the mating connector arrows not behaving as expected after the rebase

[awb1015]

I experimented with the YAML file after posting it and if I gave Connector 2 (both sides) a sequential pin list of 1,2,3,4,5,6 instead of 37,17,18,21,20,19 it seems like the issue went away. Obviously this isn't something I could do on actual diagrams as it changes the information being presented. However, I thought it might be helpful towards diagnosing the issue

[formatc1702]

Please try the updated code; your bug should be fixed in 02a800a.

Root cause:

• Use unique index for connector pin ports #229 changed the way connections between components are handled, by internally assigning unique IDs to the "ports" where wires (and now arrows) attach to connectors.
• This mapping happens inside the Harness.connect() function, which is called when attaching wires.
  Arrows between pins use Harness.add_mate_pin() instead, which did not implement the mapping
• In addition, Harness.create_graph() had to be adjusted to add +1 to convert the [0-indexed] internal IDs to 1-indexed port IDs.
  • The decision to use 1-indexed ports in GraphViz was taken so that port IDs would match the typical use case with connector pin numberings starting at 1
  • This is also the reason why the bug only appeared with your custom pin numbering.

[awb1015]

Looks good. Ran several dozens diagrams to confirm. Thank you for the update!

[kvid]

Sorry about this long period without any reply. These changes looks good. I only have a couple of suggestions.

[formatc1702]

All comments have been addressed inside #251.

Not merging yet since it would immediately push dev forward, but without the latest code suggestions. I will wait until #251 is merged to have the suggestions make it directly into dev as well.

[SeanDS]

This looks promising for my use case. Would it be possible to group together the two sides of the mate in one box, instead of those arrows? I'm thinking a box with numbering on both left and right sides and labels between indicating the name of the pin pair. As I noted in #295 there are certain connectors where pin numberings change on the same conductor, so these numbers would ideally not be fixed.

[martinrieder]

It seems that the user-configurable autogeneration character from 45bcc1d became an undocumented feature.

[kvid]

@martinrieder wrote:

It seems that the user-configurable autogeneration character from 45bcc1d became an undocumented feature.

You are right, it's not mentioned in the v0.4 docs, but I don't know if it might be due to lack of testing, unstability, side effects, just forgotten, or something else. Maybe @formatc1702 remembers any reason for omitting it from the docs?

Edit: If you @martinrieder have the time, maybe you could test this optional feature with a few different values, and verify the intended effect? Does it only work with a single character, or also with longer strings? Do you observe any unfortunate side-effects with certain characters/strings? Please store a few YAML input test cases for future regression testing.

[formatc1702]

Thanks. I have added a line in the syntax description (in the dev branch) for now so it is not forgotten, see 5905041.

It's a good idea to do some testing as suggested; then the documentation can be expanded accordingly.

[martinrieder]

@kvid wrote:

Edit: If you @martinrieder have the time, maybe you could test this optional feature with a few different values, and verify the intended effect? Does it only work with a single character, or also with longer strings? Do you observe any unfortunate side-effects with certain characters/strings? Please store a few YAML input test cases for future regression testing.

I can do that. I guess that quotation characters could be problematic in this case. For sure, there should be a check to not accept an empty string here.