Adding support for wire length units #161
Conversation
Will also be editing dataclasses.py to add the support as well as defaulting units to meters if unpopulated.
There was a problem hiding this comment.
@zombielinux 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 is currently on vacation, 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.
- Just a friendly advice: Read the hints about writing good commit messages linked from this current draft written by @formatc1702 : https://github.com/formatc1702/WireViz/blob/feature/syntax-description/docs/CONTRIBUTING.md
- Please include all commits needed for this feature into your
patch-1branch and re-push it, i.e. join (rebase or cherry-pick) the Add support for units to wire lengths. Part 2 #162 commit(s) into this PR. The automatic PR checks currently fail in this PR because yourDataClasses.pychanges are missing.
| @@ -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.lengthunit, 'designators': designators, | |||
There was a problem hiding this comment.
Here you risk making a BOM entry where total_length is the sum of several numeric values with different units. I can see at least two strategies to avoid this:
- Convert the values to a common unit before adding them together.
- Include the length unit in the
cable_groupto avoid joining entries with different length units.
I recommend discussing this choice (and if choice 1, what common unit to choose) in #7.
| f'{cable.gauge} {cable.gauge_unit}{awg_fmt}' if cable.gauge else None, | ||
| '+ S' if cable.shield else None, | ||
| f'{cable.length} m' if cable.length > 0 else None, | ||
| f'{cable.length} {cable.lengthunit}' if cable.length > 0 else None, |
There was a problem hiding this comment.
I can see an unfortunate difference in naming style between these two attribute pairs:
cable.gaugeandcable.gauge_unitcable.lengthandcable.lengthunit
Feel free to discuss naming conventions in a new issue, but I suggest we try to keep it consistent.
There was a problem hiding this comment.
added the underbar to length_unit in #171
- partial fix for wireviz#7 - based on and closes wireviz#161 and wireviz#162
- partial fix for wireviz#7 - based on and closes wireviz#161 and wireviz#162
- partial fix for wireviz#7 - based on and closes wireviz#161 and wireviz#162
- partial fix for wireviz#7 - based on and closes wireviz#161 and wireviz#162
- partial fix for wireviz#7 - based on and closes wireviz#161 and wireviz#162
|
Closed as part of #196. |
Will also be editing dataclasses.py to add the support as well as defaulting units to meters if unpopulated.