hildogjr / KiCost

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

Random problems when splitting groups of components #434

Closed set-soft closed 3 years ago

set-soft commented 3 years ago

This portion of code is wrong and introduces random behavior:

https://github.com/xesscorp/KiCost/blob/f98a646ec096d532edbe719e291a903b7c658052/kicost/edas/tools.py#L244-L245

This set is then used in:

https://github.com/xesscorp/KiCost/blob/f98a646ec096d532edbe719e291a903b7c658052/kicost/edas/tools.py#L293-L311

Which assumes the order is preserved. But a set is just a dict, so you can't expect to preserve the order. The code ends mixing the components data and the comparisson randomly fails. Is very nasty bug because it sometimes fails and others don't.

I managed to solve it using an OrderedDict:

            for f in FIELDS_MANFCAT:
                flds = collections.OrderedDict()
                for fld in [fields.get(f)]:
                    flds[fld] = True
                component_groups[h].manfcat_codes[f] = flds #set([fields.get(f)])

And then replacing:

             for f in FIELDS_MANFCAT:
-                component_groups[h].manfcat_codes[f].add(fields.get(f))
+                component_groups[h].manfcat_codes[f][fields.get(f)] = True

No patch because I must solve other problems and I must check this doesn't have side effects. But I think you'll understand what's the problem and how to solve it.

set-soft commented 3 years ago

BTW: this bug makes the code fail here:

https://github.com/xesscorp/KiCost/blob/f98a646ec096d532edbe719e291a903b7c658052/kicost/spreadsheet.py#L490-L493

Because the part.collapsed_refs is empty. I used the following patch to avoid the fail:

@@ -490,6 +490,9 @@ Yellow -> Parts available, but haven't purchased enough.''',
     # Then, order the part references with priority ref prefix, ref num, and subpart num.
     def get_ref_key(part):
         match = re.match(PART_REF_REGEX, part.collapsed_refs)
+        if match is None:
+            logger.warning('Malformed reference: `{}`'.format(part.collapsed_refs))
+            return ['?', '?', None]
         return [match.group('prefix'), match.group('ref_num'), match.group('subpart_num')]
     parts.sort(key=get_ref_key)

But I think this isn't necesary, perhaps an assert is suitable here.

set-soft commented 3 years ago

Was fixed by #436