mvnmgrx / kiutils

Simple and SCM-friendly KiCad file parser based on Python dataclasses for KiCad 6.0 and up.
GNU General Public License v3.0
81 stars 27 forks source link

Compare KiCad Library Utils with KiUtils #40

Closed eeintech closed 1 year ago

eeintech commented 1 year ago

Hello @mvnmgrx

For Ki-nTree I have been relying on KiCad Library Utils, developed and maintained by the KiCad team, to merge two symbol libraries together (eg. adding a symbol into an existing library, after updating its fields). However, this method result in improper formatting with the resulting library and I had to indicate that opening the library and saving it in KiCad's symbol editor is necessary for diff-ing.

I just found out about KiUtils and gave it a shot. Well I'm impressed as the resulting library library file has the right formatting!

Now, before to moving on and replacing the "KiCad Library Utils" with yours, I would like to understand what is different...

For my test, I used the following code with KiUtils:

from kiutils.symbol import SymbolLib

res_0 = SymbolLib.from_file('resistor_0.kicad_sym')
res_1 = SymbolLib.from_file('resistor_1.kicad_sym')

for symbol in res_1.symbols:
    res_0.symbols.append(symbol)

res_0.to_file()

and here's the code using "KiCad Library Utils":

from lib_utils.kicad_sym import KicadLibrary

res_0 = KicadLibrary.from_file('resistor_0.kicad_sym')
res_1 = KicadLibrary.from_file('resistor_1.kicad_sym')

for symbol in res_1.symbols:
    res_0.symbols.append(symbol)

res_0.write()

(lib_utils folder contains the latest sexpr.py and kicad_sym.py from KiCad Library Utils common folder)

Here are the original files (change extension back to .kicad_sym):

The result of each code snippet is:

If you have time to review this, would you let me know what you are doing differently?

Thanks!

EDIT: Found an issue with updating symbol name in Python dataset, will post a PR.

mvnmgrx commented 1 year ago

Hello, thanks for considering switching to kiutils!

Getting the output formatting right was one of my primary goals for kiutils. But functionality-wise, both libraries should do the same.

But I am still trying to understand what your actual problem here is. If i run your snippet for kiutils, i get exactly the same output as if i would just combine both parts by hand (copy&paste their symbols together). The resulting file resistor_kiutils_merge.kicad_sym can be opened in KiCad without problems.

But i noticed another thing: KiCad Library Utils seem to remove the type token of stroke (line) definitions in your paticular example: grafik (left: original, right: your output of KiCad Library Utils)

So opening files generated by KiCad Library Utils in kiutils does not work at the moment as this type token was marked mandatory in the documentation, so kiutils expects it. Despite of this minor difference, the library combined by KiCad Library Utils works fine in KiCad too.

Furthermore, when re-indenting (with tools like this) both your output files and comparing them, the only difference seems to be the placement of some tokens that are on the same indentation level (which should not cause any problems).

Best regards, M

eeintech commented 1 year ago

Hello @mvnmgrx

But I am still trying to understand what your actual problem here is

This issue is more like a discussion than a problem with kiutils :wink:

However the problem is with "KiCad Library Utils" because their indentation/formatting does not match the format of their own libraries, and for diff-ing and other library manipulation outside of the KiCad environment this creates a mess everytime. For instance adding a single resistor into a library files messes the indentation of the entire library file and then Git thinks there were thousands of changes vs just ~30 lines added.

So in this discussion I'm trying to understand what is your way of keeping the formatting right, and why is the "KiCad Library Utils" developed by the KiCad team themselves so messed up :smile:

Furthermore, when re-indenting (with tools like this) both your output files and comparing them, the only difference seems to be the placement of some tokens that are on the same indentation level (which should not cause any problems).

Is this the only magic here? You post-process the indentation after exporting the s-expr data?

mvnmgrx commented 1 year ago

Hello @eeintech

This issue is more like a discussion than a problem with kiutils 😉

Ah okay, then i was on the wrong track here :-)

messes the indentation of the entire library file and then Git thinks there were thousands of changes vs just ~30 lines added

Exactly this was the inspiration for kiutils. Using SCMs with KiCad Library Utils is just a mess ..

So in this discussion I'm trying to understand what is your way of keeping the formatting right

Is this the only magic here? You post-process the indentation after exporting the s-expr data?

There is no post-processing done. Every to_sexpr() method already outputs the S-Expression in the correct format as a string. Recursively calling this method while keeping track of the indentation level and putting together their output yields a file with correct formatting.

Take a look at this to_sexpr() for the Color token for example. KiCad Library Utils generates an intermediate list of tokens: https://gitlab.com/kicad/libraries/kicad-library-utils/-/blob/master/common/kicad_sym.py#L187-196

They then use this build_sexpr() function to generate the actual S-Expression that will be written to the output file: https://gitlab.com/kicad/libraries/kicad-library-utils/-/blob/master/common/sexpr.py#L169-L184

This is also done recursively, advancing the indentation level when further recursing down the list of tokens. KiCad seems to format things differently though, which i found out during development of kiutils is in no way consistent between tokens. They also use a lot of "one-liners" with multiple depth levels that cannot be represented this way. So KiCad Library Utils' approach will not yield correcly formatted, but still perfectly fine working S-Expression.

Now the ColorRGBA class in kituils: It returns a string representing itself in S-Expression (see line 200) without using some intermediary format like a list of tokens or some sort: https://github.com/mvnmgrx/kiutils/blob/4775576c33640ba3f61a755ca8da2729c05b4cd2/src/kiutils/items/common.py#L182-L200

This means that kiutils can generate different formatting for different tokens as a token always knows how its formatting should look like. The downside of this is that it is not generic, meaning it takes some manual work when adding or removing tokens.

So this is basically all the magic here. Just observing how KiCad does format its files and copying said format to every token's to_sexpr() function.

eeintech commented 1 year ago

Hello @mvnmgrx

Sorry for the delayed response, I'm just coming back from vacation :smile:

This is a very comprehensive answer, thanks a lot for taking the time to point out how your magic works :wink:

I am curious: why not joining the KiCad library team and put all this work into KiCad Library Utils instead of going your own way?

mvnmgrx commented 1 year ago

No problem, glad you had a nice vacation 😉

I am curious: why not joining the KiCad library team and put all this work into KiCad Library Utils instead of going your own way?

The KiCad Library Utils package did not work for me at that time (due to a bug that i found and reported, as well as the bad output formatting), and I somehow thought it was faster to just code some parser myself instead of waiting for them to fix those issues. But it actually took longer than the project i needed it for 😄

So this is why kiutils exists. I also guessed that their team wouldn't incorporate such drastic changes needed just to get the output formatting right. I mean it works just fine, why change it and possibly break something for hundreds of scripts that potentially already use KiCad Library Utils and don't care about the formatting of the files.

eeintech commented 1 year ago

I see, makes a lot of sense sometimes to just do it yourself, if you have the motivation for it! Glad you made it happen!