add length_unit support for wire and cable lengths #171

stevegt · 2020-10-05T07:34:01Z

partial fix for [feature] Using inches and other units #7
based on and closes Adding support for wire length units #161 and Add support for units to wire lengths. Part 2 #162

Will also be editing dataclasses.py to add the support as well as defaulting units to meters if unpopulated.

Will also be editing harness.py to add the support as well as defaulting units to meters if unpopulated.

- partial fix for wireviz#7 - based on and closes wireviz#161 and wireviz#162

kvid

@stevegt Thank you for contributing. I'm just another contributor, like you. @formatc1702 is the owner that approves and merges PRs into dev when he thinks they are ready. He has been on vacation and busy with other things a few weeks, and I decided to give you a few advices that I believe will help you getting this PR accepted, but remember that @formatc1702 might not share all my opinions.

Please read the hints in this current draft written by @formatc1702 : https://github.com/formatc1702/WireViz/blob/feature/syntax-description/docs/CONTRIBUTING.md
I recommend you interactively rebase your PR branch named length_unit and fix these issues:
- Execute rebase command: git rebase -i HEAD~3
- Untangle the commit history by this rebase that replaces your merge.
- Reword the first two commit messages:

reword 30ac03c Adding support for wire length units
reword 2dde3a2 Add support for units to wire lengths. Part 2
pick 684cefb add length_unit

Replace both commit messages with e.g. "Add support for wire length units" to use the imperative form and remove the "Part 2" reference. Personally, I would also consider squashing them together, i.e. write "squash" instead of "reword" for the second commit in the initial rebase step above. Take a look at the commit histories of my demonstration branches kvid:length_unit-reword and kvid:length_unit-squash to see what the results will be.

Edit: After rebasing your branch, you must force-push it (executing git push -f) to update your PR.

kvid · 2020-10-05T08:21:33Z

src/wireviz/DataClasses.py

@@ -88,6 +88,7 @@ class Cable:
    gauge_unit: Optional[str] = None
    show_equiv: bool = False
    length: float = 0
+    length_unit: Optional[str] = None


Suggested change

length_unit: Optional[str] = None

length_unit: str = 'm'

When any None value is always replaced with the same real default string value in __post_init__(), then there is no reason to have an optional type and None as default value - see why in my #156 (comment). The same effect is better obtained with the real default string value here in the declaration.

kvid · 2020-10-05T08:56:50Z

src/wireviz/DataClasses.py

+        if self.length_unit is None: #Default wire length units to meters if left undeclared
+            self.length_unit = 'm'
+


Suggested change

if self.length_unit is None: #Default wire length units to meters if left undeclared

self.length_unit = 'm'

The same effect is better obtained with the real default string value in the declaration above.

kvid · 2020-10-05T10:28:22Z

src/wireviz/Harness.py

@@ -365,7 +365,7 @@ def bom(self):
            gauge_name = f' x {shared.gauge} {shared.gauge_unit}' if shared.gauge else ' wires'
            shield_name = ' shielded' if shared.shield else ''
            name = f'Cable{cable_type}, {shared.wirecount}{gauge_name}{shield_name}'
-            item = {'item': name, 'qty': round(total_length, 3), 'unit': 'm', 'designators': designators,
+            item = {'item': name, 'qty': round(total_length, 3), 'unit': shared.length_unit, 'designators': designators,


I've already commented this code change in my #161 (comment). Choice 2 is the easiest to implement by including c.length_unit in the tuple returned from the cable_group lambda just above this loop:

cable_group = lambda c: (c.category, c.type, c.gauge, c.gauge_unit, c.length_unit, c.wirecount, c.shield, c.pn, c.manufacturer, c.mpn)

Then, remember to implement the same changes for bundle wires below as for cables here.

stevegt · 2020-10-05T20:01:16Z

@kvid Thanks for the advice; most of it looks good to me, though I'm concerned that the force push runs the risk of breaking others' repos. I also wouldn't want attribution to be lost -- a Co-authored-by header would seem to be called for if the commits are squashed, so that should probably be included. I know we're kind of stalled here due to lack of alternate committers (I vote for you!), but otherwise I'd be careful about setting the bar too high for PRs, rather than accepting as-is and then allowing others to build on that history. Normally when running a project I'd go ahead and accept these commits as-is and then solicit issues and fixes, particularly if working on a dev branch.

In my own case, I urgently needed #161 and #162 late last night, was pleasantly surprised that @zombielinux had already done enough work to satisfy the requirement I had, and was glad that he had make those commits available. His code worked perfectly for my use case, and saved me from having to duplicate his effort. I'd like to see his work included and I'd like to see him get attribution so that others can improve on it.

kvid · 2020-10-05T23:04:28Z

@kvid Thanks for the advice; most of it looks good to me, though I'm concerned that the force push runs the risk of breaking others' repos.

As long as you force-push to your own PR branch where you normally are the only one to push contributions, then I don't see what could go wrong. It has been done several times in other PRs (e.g. #163 and #164) to this project. Github discovers the force-push and handles it perfectly. If I have locally fetched commits from the PR branch of another user and he then force-push his branch after a rebase, then I still have the old commits, and when re-fetching, I also get the new commits. Normally, I then reset my local branch to the new remote branch to get in sync again, but under no circumstances I have seen anything broken because of this. Please explain to me how this can break a repo.

I also wouldn't want attribution to be lost -- a Co-authored-by header would seem to be called for if the commits are squashed, so that should probably be included.

I didn't intend to suggest removing any authorship. Squashing two commits from the same author together will keep the author unchanged - see kvid@94f752d that reflects what I suggested. I also suggested a minor rewording of the commit message to follow the commit guidelines linked from https://github.com/formatc1702/WireViz/blob/feature/syntax-description/docs/CONTRIBUTING.md. However, please feel free to ignore any suggestions you don't agree with. I don't want to, and certainly don't have the power to, insist on anything. 😃

I know we're kind of stalled here due to lack of alternate committers (I vote for you!), but otherwise I'd be careful about setting the bar too high for PRs, rather than accepting as-is and then allowing others to build on that history. Normally when running a project I'd go ahead and accept these commits as-is and then solicit issues and fixes, particularly if working on a dev branch.

As I tried to explain in my review message, that I decided to write a code review and suggest changes that I believe will bring the PR closer to a state where it has the intended functionality and follow most of the project guidelines, but @formatc1702 is the owner that decides what PRs are accepted into dev. My experience from earlier PRs tells me he seems to prefer waiting until the PR has reached a state where the code is working as intended without any known major issues and that the source code is somewhat readable before accepting it. He also seems to prefer a linear commit history that follows his guidelines and squashing most intermediate commits together before accepting it and then rebase it on top of dev.

One of the reasons for me to suggest changes rather that fix it myself afterwords, is to enable the PR author to keep his authorship on all code that he changes, but he can of course suggest that someone else can do the remaining work if he really wants that, e.g. because he is getting too busy himself.

In my own case, I urgently needed #161 and #162 late last night, was pleasantly surprised that @zombielinux had already done enough work to satisfy the requirement I had, and was glad that he had make those commits available. His code worked perfectly for my use case, and saved me from having to duplicate his effort. I'd like to see his work included and I'd like to see him get attribution so that others can improve on it.

I fully agree with your opinion here. My intention with the suggestions are here ordered by importance:

Remove the bug that might lead to adding two length values of different units.
Completing the same new functionality for bundle wires as for cables.
As @formatc1702 seems to prefer rebase before merge when joining commits from different branches, I suggest following that convention, but feel free to ignore this.
Squashing the two commits into one was suggested due to MHO that they should have been committed together in the first place because none of them can work without the other, but others might disagree.

You decide what to include in your PR, and @formatc1702 decide what to merge into his project.

formatc1702 · 2020-10-23T14:36:34Z

I'm late to the party, but here are my comments :)

Re: preserving authorship

@stevegt:
I also wouldn't want attribution to be lost -- a Co-authored-by header would seem to be called for if the commits are squashed, so that should probably be included.

@kvid:
I didn't intend to suggest removing any authorship. Squashing two commits from the same author together will keep the author unchanged - see kvid/WireViz@94f752d that reflects what I suggested.

@kvid:
One of the reasons for me to suggest changes rather that fix it myself afterwords, is to enable the PR author to keep his authorship on all code that he changes, but he can of course suggest that someone else can do the remaining work if he really wants that, e.g. because he is getting too busy himself.

I'm still figuring out managing authorships when merging & squashing, but it seems that I have control over it by adding the Co-Authored-By: header as @stevegt suggested. So no need to restructure the commit history or worry about losing @zombielinux as a co-author, I will keep it in mind.

Side note:
I made some tests, involuntarily involving @kvid in commits, as well as in a squash+merge, without him even knowing this was happening 😄 sorry again for appropriating your username there. The point is, no matter what happens, authors will get attribution.

Re: when to close PRs

@stevegt:
I'd be careful about setting the bar too high for PRs, rather than accepting as-is and then allowing others to build on that history. Normally when running a project I'd go ahead and accept these commits as-is and then solicit issues and fixes, particularly if working on a dev branch.

@kvid:
My experience from earlier PRs tells me he seems to prefer waiting until the PR has reached a state where the code is working as intended without any known major issues and that the source code is somewhat readable before accepting it. He also seems to prefer a linear commit history that follows his guidelines and squashing most intermediate commits together before accepting it and then rebase it on top of dev.

There is no formalized process, or checklist to decide when a PR should be merged.
However, as @kvid's code review points out, there are still some easy improvements that can be made (including the ones referencing comments #161), so there is no reason to rush a merge before fixing already known issues.
I have come to trust @kvid's code review skills, so I tend to wait for his green light before merging. This includes marking all code review comments as resolved; either by implementing them, or by argumenting against them and rejecting them, which rarely happens.
As the repo owner, of course I am free to ignore any external advice here, but so far, collaboration has been great overall.

@stevegt:
In my own case, I urgently needed #161 and #162 late last night, was pleasantly surprised that @zombielinux had already done enough work to satisfy the requirement I had, and was glad that he had make those commits available. His code worked perfectly for my use case, and saved me from having to duplicate his effort. I'd like to see his work included and I'd like to see him get attribution so that others can improve on it.

That's the beauty of distributed version control :)
Submitting PRs early (as drafts if necessary) is great, and I do it too, to transparently show what we are working on and making the code available. This allowed you to merge the PRs you needed into your own copy, while the main repo can wait for more thorough review before deciding to merge into dev.

Moving forward

Since I've figured out how to preserve co-authorship, it is up to @stevegt and @kvid to decide who will implement the proposed changes... in @kvid's case he would have to submit a new PR, and I would make sure to credit everyone involved upon merging.
Like I said, no need to clean up the commit history since everything will be squashed by me before merging.
So let me know!

Also, I haven't gotten around to testing this PR myself, so expect some review comments from me as well.

Thanks for your contribution!

formatc1702 · 2020-10-23T15:05:12Z

There's one request that I would like to note. This can also become a separate PR if so desired.

A way to parse the length attribute and extract any unit from it automatically, similar to how gauge supports <float> mm2, <int> AWG and <int|float> <randomunit> in addition to normal ints and floats, which default to mm2.
So setting length: 2 in in the YAML should set length = 2 and length_unit = 'in' within the dataclass properties.
The unit should still default to m if only a number is specified, like it does now.

kvid · 2020-10-31T12:12:22Z

There's one request that I would like to note. This can also become a separate PR if so desired.

A way to parse the length attribute and extract any unit from it automatically, similar to how gauge supports <float> mm2, <int> AWG and <int|float> <randomunit> in addition to normal ints and floats, which default to mm2.
So setting length: 2 in in the YAML should set length = 2 and length_unit = 'in' within the dataclass properties.
The unit should still default to m if only a number is specified, like it does now.

I agree with this request, but I suggest raising it as a new issue to avoid delaying this PR even more. As @formatc1702 wants to squash this PR, then these issues are remaining:

Fix the bug
Handle bundles
Resolve any conflicts with the current dev (might increase over time as other PRs are merged in)

@stevegt do you want to do this soon, or do you want e.g. me to finish the task?

formatc1702 · 2020-10-31T16:32:11Z

I agree with this request, but I suggest raising it as a new issue to avoid delaying this PR even more.

👍 Will probably implement this (auto-parsing length and setting length_unit) myself and submit my own PR after this one is merged.

Based on #161, #162, #171. Co-authored-by: stevegt <stevegt@t7a.org> Co-authored-by: kvid <kvid@users.noreply.github.com>

formatc1702 · 2020-11-16T16:23:39Z

Closed as part of #196.

Based on #161, #162, #171. Co-authored-by: stevegt <stevegt@t7a.org> Co-authored-by: kvid <kvid@users.noreply.github.com>

zombielinux and others added 4 commits August 28, 2020 18:00

Adding support for wire length units

30ac03c

Will also be editing dataclasses.py to add the support as well as defaulting units to meters if unpopulated.

Add support for units to wire lengths. Part 2

67a25e3

Will also be editing harness.py to add the support as well as defaulting units to meters if unpopulated.

Merge remote-tracking branch 'zombielinux/patch-2' into length_unit

14c6103

add length_unit

32cb73b

- partial fix for wireviz#7 - based on and closes wireviz#161 and wireviz#162

stevegt mentioned this pull request Oct 5, 2020

Adding support for wire length units #161

Closed

kvid requested changes Oct 5, 2020

View reviewed changes

stevegt mentioned this pull request Oct 7, 2020

[feature] Export technical drawing to PDF, with frame, title block, BOM #32

Open

formatc1702 mentioned this pull request Nov 14, 2020

[meta] Collaborator discussion #101

Open

kvid mentioned this pull request Nov 16, 2020

Length unit #196

Merged

formatc1702 pushed a commit that referenced this pull request Nov 16, 2020

Add support for wire length units

dec64ab

Based on #161, #162, #171. Co-authored-by: stevegt <stevegt@t7a.org> Co-authored-by: kvid <kvid@users.noreply.github.com>

formatc1702 closed this Nov 16, 2020

formatc1702 added a commit that referenced this pull request Nov 16, 2020

Add #7, #196 (#161, #162, #171)

4fd8b1f

formatc1702 pushed a commit that referenced this pull request Nov 17, 2020

Add support for wire length units

3c205fe

Based on #161, #162, #171. Co-authored-by: stevegt <stevegt@t7a.org> Co-authored-by: kvid <kvid@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add length_unit support for wire and cable lengths #171

add length_unit support for wire and cable lengths #171

stevegt commented Oct 5, 2020

kvid left a comment •

edited

kvid Oct 5, 2020

kvid Oct 5, 2020

kvid Oct 5, 2020

stevegt commented Oct 5, 2020

kvid commented Oct 5, 2020 •

edited

formatc1702 commented Oct 23, 2020 •

edited

formatc1702 commented Oct 23, 2020 •

edited

kvid commented Oct 31, 2020

formatc1702 commented Oct 31, 2020 •

edited

formatc1702 commented Nov 16, 2020

		if self.length_unit is None: #Default wire length units to meters if left undeclared
		self.length_unit = 'm'

add length_unit support for wire and cable lengths #171

add length_unit support for wire and cable lengths #171

Conversation

stevegt commented Oct 5, 2020

kvid left a comment • edited

Choose a reason for hiding this comment

kvid Oct 5, 2020

Choose a reason for hiding this comment

kvid Oct 5, 2020

Choose a reason for hiding this comment

kvid Oct 5, 2020

Choose a reason for hiding this comment

stevegt commented Oct 5, 2020

kvid commented Oct 5, 2020 • edited

formatc1702 commented Oct 23, 2020 • edited

Re: preserving authorship

Re: when to close PRs

Moving forward

formatc1702 commented Oct 23, 2020 • edited

kvid commented Oct 31, 2020

formatc1702 commented Oct 31, 2020 • edited

formatc1702 commented Nov 16, 2020

kvid left a comment •

edited

kvid commented Oct 5, 2020 •

edited

formatc1702 commented Oct 23, 2020 •

edited

formatc1702 commented Oct 23, 2020 •

edited

formatc1702 commented Oct 31, 2020 •

edited