-
Notifications
You must be signed in to change notification settings - Fork 0
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
Combine Altium multi-part components #147
Conversation
4384260
to
eb842b5
Compare
afb5993
to
6fe9d6c
Compare
combined_components.append(component) | ||
|
||
for designator, multi_part_components in multi_part_components_by_designator.items(): | ||
combined_component = multi_part_components[0].copy() |
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.
There is an interesting side effect of this statement. We're taking the attributes of the "first" part as the attributes of the combined component. But there can be cases where the attributes differ. In Archimajor, there are some places where the first part has no attributes but some other part does. I don't think there is a reasonable and straightforward solution here for that case, which I think should be considered an issue on the board.
6fe9d6c
to
1d274d0
Compare
1d274d0
to
382a9d2
Compare
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.
💯
@@ -54,6 +60,8 @@ def list_components( | |||
applies to Altium projects. | |||
:param ref: Optional git ref to check. This can be a commit hash, branch | |||
name, or tag name. Default is "main", i.e. the main branch. | |||
:param combine_multi_part: If True, multi-part components will be combined | |||
into a single component. Currently only works for Altium. |
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.
I don't even think this needs to be a parameter. I can't think of a reason someone wouldn't want to combine these. We typically refer to them as multi-symbol components here, as they really are one part, just separated logically to make design organization easier.
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.
This is separated out in list_components
because you might want to check each part separately, such as in the case when you want to see if all parts of a component are placed, where they are placed etc. in the BOM they are always combined.
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.
Ah, I see 👍
@kdumontnu Should I tag Alex in for the second review? |
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.
👍
Replaces #124 and closes #146.
Uses the new IR changes in Tools to more accurately and correctly combine multi-part components. This PR will remain in draft until these Tools changes are deployed to production first.
Stacked on #145 to benefit from those changes.
Outdated
Todo: