-
Notifications
You must be signed in to change notification settings - Fork 212
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
Large scale refactoring #251
base: dev
Are you sure you want to change the base?
Conversation
b3fd091
to
b45d0ae
Compare
Dear Daniel, I just wanted to drop you a line that I appreciate your efforts on this task very much. With kind regards, |
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
Thank you for your patience and understanding. |
d69e3ba
to
417c842
Compare
@formatc1702 wrote:
I'd be happy to do that, but the installation failes as described in #287.
I agree this sounds like a wise decision.
OK |
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. |
I've not yet had time to test all new code, but I list here a few issues I've noticed so far:
|
The idea is that I agree that testing |
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 This will run the installation process in development mode (what was 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
[1] https://github.com/formatc1702/WireViz/blob/master/docs/README.md#installing-the-development-version |
Allow absent "prefix" in group entries to simplify the code
I pushed yesterday a couple of commits I'm using to:
I'm currently investigating some strange generated HTML. |
I think the biggest issues that should probably remain inside the scope of this PR (based on a project-wide search for
Overall I agree with @amotl 's and @kvid 's assessments. All other |
Bug: 0x112233:0x445566 in YAML input didn't convert such colors to #112233:#445566 and the strings where just passed as uppercase to the .gv file. Hence Graphviz printed warnings about unknown colors and used black as color instead. Add test for int as string. Re-ordered if statements to give an exception when a color has an unknown type.
1944b19
to
ca7b134
Compare
amount: Optional[NumberAndUnit] = None | ||
sum_amounts_in_bom: bool = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me how to use amount
and sum_amounts_in_bom
. They are not mentioned in syntax.md
and not used in any example/tutorial/test harness. I hope @formatc1702 can provide some examples.
If these and qty
are not supported in the Connector
class, why are they all defined in this common superclass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion. It stems from a wish to be able to specify how to handle lengths in the BOM.
Example:
A harness contains two identical cables of 1m each, another cable of same type but 5m, and two bundles of four identical wires of 1m each.
sum_amounts_in_bom = True
would show the BOM requiring 7m of cable + 8m of wire
(that is the current behavior)sum_amounts_in_bom = False
would show the BOM requiring 2 pcs. of cable (1m) + 1 pcs. of cable (5m) + 8 pcs. of wire (1m)
In a workflow where the person asssembling the harness is expecting to cut the cables themselves, option 1 is fine. If they are expecting to receive all cables/wires cut to length, option 2 is the way to go.
Another typical example is heatshrink: If there is a connector that requires 10mm of heatshrink on each of its 15 pins, and 30mm of the same heatshrink on the shield, the BOM could show either
- 180mm of heatshrink, or
- 15 pcs. of 10mm heatshrink + 1 pc. of 30mm heatshrink, which is much more understandable in context.
This is where the amount
property comes in. The heatshrink would be defined as an additional component with qty: 15
and amount: 10 mm
, and a separate additional component would have qty: 1
and amount: 30 mm
.
If these and
qty
are not supported in theConnector
class, why are they all defined in this common superclass?
Because they are also intended for use with AdditionalComponent
, as in the heatshrink example.
#251 (comment) No output changed for any examples/tutorial/tests input.
3560766
to
4adad9d
Compare
Removed from `Connector` class since it is already defined in the `Component` superclass.
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:
Issues being addressed/solved:
A better description will follow once the code is presentable.
I am sharing the work in progress for transparency's sake.