search

KiCad / kicad-library-utils

Some scripts for helping with library development
GNU General Public License v3.0
128 stars 92 forks source link

S4.3: Some errors were detected yet a 0 code was returned #268

Closed diegoherranz closed 4 years ago

diegoherranz commented 5 years ago

Issue found on https://github.com/KiCad/kicad-symbols/issues/1114

errorExtra() doesn't mean non-zero return code unless preceded by an error(). So, a few errorExtra() calls now using error() instead.

I've also added a second commit with some code style fixes. Let me know if you disagree with any of that and I can change or remove it. Edit: I've removed that second commit and moved the style fixes to a different PR (https://github.com/KiCad/kicad-library-utils/pull/269) since they are separate things. It also includes fixes on many other files now.

Thanks!

diegoherranz commented 5 years ago

Gentle reminder. This can improve the checking quality for stacking pins and avoid new problems on the library. Thanks!

poeschlr commented 5 years ago

I would assume the error extra stuff is what would normally be printed only with the -v options. Is this correct? (meaning the stuff that is reported below a more generic error.)

So might it be better to have one common error that is reported if any of these is violated and keep the current more detailed report as error extra?

Edit: Might this be something that needs to be done in the errorExtra functionality?

diegoherranz commented 5 years ago

errorExtra() is only shown if using "double verbose" -vv.

I guess I could add some error() messages and leave the errorExtra() ones as they were. However, the way the code is, I would need to add an error() for each errorExtra() I have modified since I can't add a generic error() above which is common for them all. Also it seems that we report quite a few error() with similar level of detail already: Examples: https://github.com/KiCad/kicad-library-utils/blob/master/schlib/rules/S4_3.py#L100 or https://github.com/KiCad/kicad-library-utils/blob/master/schlib/rules/S4_3.py#L109

Either way let me know and I'll change it or just fix the merge conflict.

Thanks!

P.S.: Just for reference, when Travis checks the symbol libraries, it runs comparelibs.py which runs checklib.py using -vv. (https://github.com/KiCad/kicad-library-utils/blob/master/schlib/comparelibs.py#L56)

poeschlr commented 4 years ago

I am ok with it as long as there is still reasonable output even without the -v options. So please rebase.

diegoherranz commented 4 years ago

Rebased to master. Thanks!

poeschlr commented 4 years ago

Thanks