Empty "kicost.<variant>:dnp" field does not enable component.

[mdeweerd]

I created test case "variants_3.xml" to be run with "variant1".

https://github.com/mdeweerd/KiCost/tree/variants_3

It has:

    <comp ref="C4">
      <value>4nF</value>
      <datasheet>~</datasheet>
      <fields>
        <field name="desc">Placed only when variant1</field>
        <field name="manf">defaultmanf</field>
        <field name="kicost.variant1:dnp"></field>
        <field name="dnp">1</field>
      </fields>
    </comp>

When "variant1" is chose, the component C4 does not appear in the output. However, as the field 'dnp' should be empty in 'variant1', C4 should appear in the output.

[I am now adding an example for a modified empty comment].

[mdeweerd]

Empty description in variant leads to "None" in CSV file - not sure if this should be "None" or empty field:

C8,8nF,None,,defaultmanf,,100,,0

[mdeweerd]

I updated 'test_kicost.py' so that it integrated variants_3 in the branch mentioned above (it also makes test_kicost.py independant from the location where it was run, I made a separate pull request for that).

[set-soft]

When "variant1" is chose, the component C4 does not appear in the output. However, as the field 'dnp' should be empty in 'variant1', C4 should appear in the output.

This is because dnp is defined after kicost.variant1:dnp. I agree this shouldn't matter, but KiCad EDA import module is coded this way. This code should be outside KiCad module and affect other modules. So I'll see how to move the code first.

[mdeweerd]

This is because dnp is defined after kicost.variant1:dnp. I agree this shouldn't matter, but KiCad EDA import module is coded this way. This code should be outside KiCad module and affect other modules. So I'll see how to move the code first.

Which is why the test case was built like this ;-). It's also the reasonf for https://github.com/xesscorp/KiCost/blob/8a153a5db6f4598d75d07f9481c32c75291b9ea5/kicost/edas/eda_kicad.py#L52 but that isn't applying to the dnp field I guess.

[set-soft]

The test is controversial, you are assigning nothing to kicost.variant1:dnp and KiCost discards some of these cases with a comment

Do not create empty fields. This is useful when used more than one `manf#` alias in one designator.

Don't know what exactly means, but with the same criteria your variant overwrite could be discarded. The docs says Setting the value of this field to a non-zero number or any string will cause this part to be omitted. Now: nothing at all is included in "any string"?

I think we need to clarify both questions.

[mdeweerd]

IMHO, in KiCAD default component fields can be set and these will always show up as fields for the component. In KiCAD, an empty field would not be an active field in the eyes of the user. So extending this to the dnp field, an empty dnp field should be interpreted as an inactive field.

The component C4 in the test cas is said to be "Placed only when variant1" in the description. Which is the intention:

• In case "variant1" is not set, the 'dnp' field is 1, so the component is not placed;
• In case "variant1" is set, the 'dnp' field is empty, so the component is placed.

Empty fields specified by variants should take precedence over less specific fields.

[set-soft]

I agree, but don't know what this comment is saying:

Do not create empty fields. This is useful when used more than one `manf#` alias in one designator.

My current implementation (not yet commited) does what you say: Empty fields specified by variants should take precedence over less specific fields.

But I don't know what will break if you use kicost.VARIANT:manf# empty.

[mdeweerd]

The comment appeared three years ago: https://github.com/xesscorp/KiCost/blame/df700a0def22819f3accb41279e7ed122a2a2ce7/kicost/edas/eda_kicad.py .

The comment does not say "do not create empty manf# fields" .

The 'manf#' can be set through different field names, according to the documentation:

The allowable field names for part numbers are:

mpn          pn           p#
part_num     part-num     part#
manf_num     manf-num     manf#
man_num      man-num      man#
mfg_num      mfg-num      mfg#
mfr_num      mfr-num      mfr#
mnf_num      mnf-num      mnf#

So in that case, an empty field should not suppress another base field.

With kcost.:manf# fields the user is not getting any of the "default" fields from the libraries and is setting this option explicitally in his design. So if the explicit optioin is to have an empty field, then this explicit option takes precedence.

Also, we now have a working test set up. That means that if this breaks expected behavior, it should be noticed by a test case. If not, the test case either does not exist, or the test case is not broken.

[mdeweerd]

There should be a test case where components have all of these fields an only one of them filled. so a test case with 21 components and each component having one of mpn, pn, etc set.

[set-soft]

With kcost.:manf# fields the user is not getting any of the "default" fields from the libraries and is setting this option explicitally in his design. So if the explicit optioin is to have an empty field, then this explicit option takes precedence.

It makes sense.

Also, we now have a working test set up. That means that if this breaks expected behavior, it should be noticed by a test case. If not, the test case either does not exist, or the test case is not broken.

Lamentably most of the tests doesn't explain their purpose and tests only a small part of the functionality over and over.

[set-soft]

There should be a test case where components have all of these fields an only one of them filled. so a test case with 21 components and each component having one of mpn, pn, etc set.

I'm adding a very simple test, not the 21 cases.

[set-soft]

I'm closing it, but if you find that variant_3 is wrong please reopen it.

[mdeweerd]

C4,4nF,Placed only when variant1,,,defaultmanf,,100,,0

is missing in tests/expected_test/variants_3_variant1.csv

@set-soft (I can not reopen the "Issue" - at least I do not fin dhte function in my interface).

[mdeweerd]

@set-soft I created the attached script to generate the test case for the 21 accepted field names for the "manf#" fields, and at the same time comments.

I propose to add it to a subdirectory "tools" in the "tests" directory.

It could be useful as a starting point to generate other tests.

genPartsAndCommentsTest.txt

[set-soft]

C4,4nF,Placed only when variant1,,,defaultmanf,,100,,0

is missing in tests/expected_test/variants_3_variant1.csv

This is again the problem I mentioned before. Is an empty string "any string"? My intuition say no. In fact I thought this way when I created the variant_1 test. I assigned " " to the field. To my surprise it was interpreted as a valid string. Note that spaces must be removed, people doesn't see them and thinks they aren't there.

I'm changing the behavior and documenting it. But I'm not sure about it. Just doing it because I don't use it and you do. IMHO you should do the overwrite using "0" not "".

@set-soft (I can not reopen the "Issue" - at least I do not fin dhte function in my interface).

Sorry, it looks like only people with special permissions can do it from this page. I see the button.

[set-soft]

I propose to add it to a subdirectory "tools" in the "tests" directory.

It could be useful as a starting point to generate other tests.

genPartsAndCommentsTest.txt

I put it just in /tools we have only a couple of them.