Skip to content

Fix DNP and variant fields being ignored with field case normalization - #588

Open
DominikPalo wants to merge 1 commit into
openscopeproject:masterfrom
DominikPalo:fix-normalized-dnp-variant-fields
Open

DominikPalo wants to merge 1 commit into
openscopeproject:masterfrom
DominikPalo:fix-normalized-dnp-variant-fields

Conversation

@DominikPalo

Copy link
Copy Markdown
Contributor

With "Normalize field name case" / --normalize-field-case enabled, EcadParser.normalize_field_names lowercases the keys of each component's extra fields (e.g. DNP → dnp), but the field names offered in the dialog and passed via --dnp-field / --variant-field keep their original case. generate_bom already accounts for this when looking up shown fields (field.lower()), but two other lookups did not.

skip_component (core/ibom.py)

It looked up config.dnp_field and config.board_variant_field as-is, so with normalization on:

  • parts with the DNP field set stayed in the BOM,
  • a variant whitelist excluded every part,
  • a variant blacklist excluded nothing.

Both names are now lowercased before the lookup when normalize_field_case is set. --variant-field defaults to None rather than '', so the lowercasing is guarded.

FieldsPanel.OnBoardVariantFieldChange (dialog/settings_dialog.py)

It had the same mismatch when collecting the values of the selected variant field, so the whitelist/blacklist choices came up empty with normalization on. The selection is now lowercased too.

Normalization also drops empty values (remap keeps only truthy values). In normalized mode the dialog therefore treats a missing key as empty and offers <empty>, which matches how skip_component treats such components. With normalization off, behavior is unchanged.

Testing

Copy of the KiCad Arduino_Uno template with fields added via pcbnew: DNP=yes on J2, and Variant = A / B / A / (empty) on J1 / J2 / J3 / J4. CLI run with KiCad's Python, mounting holes blacklisted:

Options master this PR
--dnp-field DNP J2 skipped J2 skipped (unchanged)
--dnp-field DNP --normalize-field-case J1, J2, J3, J4 in BOM J2 skipped
--variant-field Variant --variants-whitelist A --normalize-field-case empty BOM J1, J3
--variant-field Variant --variants-blacklist B --normalize-field-case nothing excluded J2 excluded
--variant-field Variant --variants-whitelist "A,<empty>" --dnp-field DNP J1, J3, J4 unchanged

Dialog: I instantiated the real SettingsDialog with KiCad's wxPython, loaded the board as extra data, selected Variant as the variant field and read the whitelist items:

master this PR
normalize off <empty>, A, B <empty>, A, B
normalize on (empty list) <empty>, A, B

Not addressed here: when the dialog opens, set_extra_data_path parses the extra data before transfer_to_dialog restores a saved "normalize case" setting. That is a separate ordering issue.

🤖 Generated with Claude Code

With normalize_field_case, extra field names are lowercased in the
component data, but the dialog and --dnp-field / --variant-field keep
the original case (e.g. "DNP"). skip_component looked the names up
as-is, so DNP parts stayed in the BOM, a variant whitelist excluded
everything and a variant blacklist did nothing. Lowercase both names
before the lookup, like generate_bom already does for shown fields.

The settings dialog had the same problem when listing the values of
the selected variant field, so the whitelist/blacklist choices came up
empty. Lowercase the selection there too, and since normalization
drops empty values, offer <empty> for components without the field,
matching how skip_component treats them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment on lines +370 to +371
if selection in field_dict or normalize:
v = field_dict.get(selection, "")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't invent empty value if field is not set.

This branch has not been deployed

No deployments
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.

2 participants