Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use unique index for connector pin ports #229

Merged
merged 6 commits into from
Aug 23, 2021

Conversation

formatc1702
Copy link
Collaborator

@formatc1702 formatc1702 commented Mar 23, 2021

Closes #160.

Use unique numerical index instead of string (which might cause issues when using pin names that are not unique when ignoring upper/lower case).

This does not address the issue of having special HTML characters as part of pin names, labels, etc. as mentioned in #160 and now listed as #230; there should be a separate PR dealing with escaping these entities in the GraphViz HTML (e.g. > to >).

@formatc1702 formatc1702 force-pushed the feature/pin-case-sensitivity branch 4 times, most recently from 29b5e36 to de7f0d8 Compare March 23, 2021 22:12
Use unique numerical index instead of string (which might cause issues when using pin names that are not unique when ignoring upper/lower case).
Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your implementation with a few simple changes to obtain unique port numbers, but I also suggest using one-based port numbers to keep them identical to the pin numbers in the typical case as you describe in #160 (comment). Adding +1 at four locations is enough to ensure this.

If you agree to this, it might also be wise to name the variables pinindex (or pinidx if it must be short) instead of pinid when they contain zero-based pin index numbers. What do you think?

I also suggest updating the type hints of the Cable.connect() parameters and the Connection attributes to reflect their new pin index usage:

PinIndex = int  # Zero-based pin index
NoneOrMorePinIndices = Union[PinIndex, Tuple[PinIndex, ...], None]  # None, one, or a tuple of zero-based pin indices

src/wireviz/DataClasses.py Show resolved Hide resolved
src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/Harness.py Show resolved Hide resolved
src/wireviz/Harness.py Show resolved Hide resolved
formatc1702 and others added 3 commits August 22, 2021 19:01
so that in the standard case, internal pin IDs match the actual connector's pin numbers, which usually start from 1.

Co-authored-by: kvid <kvid@users.noreply.github.com>
as suggested by @kvid.

Co-authored-by: kvid <kvid@users.noreply.github.com>
as suggested by @kvid.

Co-authored-by: kvid <kvid@users.noreply.github.com>
@formatc1702 formatc1702 requested a review from kvid August 22, 2021 17:09
@formatc1702
Copy link
Collaborator Author

I have implemented your suggestions, please have a look at the updated code.

@kvid
Copy link
Collaborator

kvid commented Aug 23, 2021

My change suggestions have been implemented, and the test cases mentioned in #160 seems to be handled as expected now. When building examples, the ex08.gv and ex09.gv files are then only different files because their internal port lables are numbered differently. However, all PNG files are still equal, as expected.

The change will instead be logged in the `doc/changelog` branch
@formatc1702 formatc1702 merged commit e212fc9 into dev Aug 23, 2021
@formatc1702 formatc1702 deleted the feature/pin-case-sensitivity branch August 23, 2021 17:26
formatc1702 added a commit that referenced this pull request Aug 23, 2021
formatc1702 added a commit that referenced this pull request Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants