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

Remove non-BOM components from Altium BOM #119

Merged
merged 3 commits into from
May 29, 2024
Merged

Remove non-BOM components from Altium BOM #119

merged 3 commits into from
May 29, 2024

Conversation

shrik450
Copy link
Collaborator

@shrik450 shrik450 commented May 20, 2024

Closes #82.

Uses the component kind of an altium component to determine if it shouldn't be in the BOM. This is configurable and defaults to not removing components to keep the default output the same before and after this change. We can possibly change this to remove by default, but that would be a breaking change. This has been changed to remove components by default, see #119 (comment).

Outdated

This is currently a draft as the associated change in Tools has to be deployed to the docker image so I can write a test that won't fail in CI.

What's left:

  • Test

@shrik450
Copy link
Collaborator Author

shrik450 commented May 21, 2024

This is configurable, and defaults to not removing components to keep the default output the same before and after this change

Expanding on this a little:

Before this PR, if you called generate_bom on a project that had components which were of the No BOM type, we would still include those in the BOM.

With this PR, we have two choices:

  1. We remove No BOM components by default, and let users disable the filtering if they want to;
  2. We keep No BOM components by default, and let users enable the filtering if they want to.

The advantage of (1) is that this is what most users will expect from BOM generation, as components of No BOM types shouldn't be on the BOM. But the disadvantage is that this is a breaking change: if you ran the same action with the same function in with v3.0 and this PR, you would get different results.

The advantage of (2) is that it is not a breaking change, but the behavior of generate_bom is counter intuitive.

Another option is that we could have a different default for the generate-bom packaged action. For example, if you use the action in a workflow, it could set the default to True instead of False, and so we don't have to make a breaking change in py-allspice and isolate the breaking change to the action. The disadvantage here is that the differing defaults could confuse users who go from using the action to writing their own script that depends on the library. I think that would be a relatively rare case, but it still is a problem.

@jtran do you have any preferences here?

@shrik450 shrik450 marked this pull request as ready for review May 28, 2024 19:33
@shrik450 shrik450 requested a review from a team as a code owner May 28, 2024 19:33
@shrik450 shrik450 requested review from jtran and kdumontnu May 28, 2024 19:33
@jtran
Copy link
Contributor

jtran commented May 28, 2024

The advantage of (1) is that this is what most users will expect from BOM generation, as components of No BOM types shouldn't be on the BOM.

If it's expected by the majority of users to remove No BOM components, we should do that.

But the disadvantage is that this is a breaking change

Arguably, it's a bug fix. But if you want to be stricter about it, why not bump the major version to signal that it's a breaking change?

@shrik450
Copy link
Collaborator Author

I've toggled the default. After thinking about it, I agree that it's a bugfix, and the output format removing components that never should've been there shouldn't be "breaking". This will come up again soon with multi-symbol components.

Copy link
Contributor

@kdumontnu kdumontnu left a comment

Choose a reason for hiding this comment

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

🙌

@shrik450 shrik450 merged commit c193c42 into main May 29, 2024
4 checks passed
@shrik450 shrik450 deleted the su/altium-kind branch May 29, 2024 21:39
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.

Use Component Properties for Fitted vs not-fitted in BOM Generation
3 participants