hildogjr / KiCost

Build cost spreadsheet for a KiCad project.
MIT License
520 stars 97 forks source link

Fix #370 - Variant handling for "kicost.<variant>:field" fixed #373

Closed mdeweerd closed 4 years ago

mdeweerd commented 4 years ago

Fix for #370

hildogjr commented 4 years ago

('manf#', 'manf', 'desc', 'value', 'comment', 'S1PN', 'S1MN', 'S1PL', 'S2PN', 'S2MN', 'S2PL') is an specific case yours, it not possible to merge this code. The ('manf#', 'manf', 'desc', 'value') is correct, but the other field should be taken by some internal definition/variables.

hildogjr commented 4 years ago

Close due not fix in general use.

mdeweerd commented 4 years ago

It's a pity that all code changes are thrown away this way - it suffices to ignore the change on line 100 (in the new version).

hildogjr commented 4 years ago

Doing it the variant problem is fixed?

mdeweerd commented 4 years ago

Yes, the variant problem is fixed by applying the variant regex independently on the variant part itself (and not "integrated" in the total regex where "^" and "$" of the variant regex no longer apply).

Regarding the limitation to "('manf#', 'manf', 'desc', 'value')" only, I am not sure that this test makes sense here - at leaste the way it is used anyway.

The code first tests to see if there is a "variant" expression using a regex like 'kicost(\.(?P<variant>.*))?:(?P<name>.*)'

This can match kicost.<variant>:comment for instance, but also kicost:comment .

If the field is defined to be variant dependent, then it should override the normal field if the variant is matched.

The test name not in ('manf#', 'manf') and name[:-1] not in distributor_dict supposes that name[:-1] is a distributor, but there is no test that the last character of "name" is actually a '#' token which is the designated way to indicate that this modifier is for a distributor. The test for '#' as the last character of "name" should be added, and the only exception would be "manf#" in that case.

johnthagen commented 4 years ago

FYI, CI failures are fixed in #379