protofire/solhint

Non-zero exit code breaks CI compatibility

madlabman opened this issue · 13 comments

madlabman commented

Since the version 4.5.0 there's a zero exit code (see #554) even with reported errors. It makes it hard to use within actions in CI. My personal opinion is to have the similar behaviour other linters preserve - return 1 in case of errors.

I assume the implemented change was an attempt to fix kinda niche issue (even it has fixed my workflow as well), and it was done in bad manner, I believe. My suggestion is to use command line flags to achieve the desired behaviour, such as --exit-code (to return non-zero exit code back) or, which is even better, to re-introduce the non-zero exit code and use a flag to change the behaviour on demand.

dbale-altoros commented

I will check this as soon as i can
thanks @madlabman for the suggestion

  • 🚀2
juanpcapurro commented

FWIW I overhauled the exit codes on solhint-community with:

solhint-community/solhint-community#134 -- to exit early and with a different code if there's a misconfiguration
solhint-community/solhint-community#76 -- to have --quiet not override --max-warnings

dbale-altoros commented

FWIW I overhauled the exit codes on solhint-community with:

solhint-community/solhint-community#134 -- to exit early and with a different code if there's a misconfiguration solhint-community/solhint-community#76 -- to have --quiet not override --max-warnings

cool!
Thanks @juanpcapurro for that!

MaxenceAdnot commented

Hello @dbale-altoros

Do you plan to implement this in solhint or should we move to solhint-community ?

dbale-altoros commented

Hey @MaxenceAdnot I will implement this fix as well
Probably next week
solhint community is a fork from protofire solhint
both are open source

We will mantain this original solhint because it is an important part of protofire products

  • 👍2
dbale-altoros commented

@MaxenceAdnot
@madlabman
Expect new solhint version on Monday

  • 👍1
dbale-altoros commented

Will merge this
#578
To fix this issue

  • 🎉1
madlabman commented

image
Welp, still doesn't work for me.

Probably, the reason is the following code

if (reports[0].errorCount > 0) process.exit(EXIT_CODES.REPORTED_ERRORS)

should iterate over the all the reports instead.

@dbale-altoros

dbale-altoros commented

@madlabman
Yes , sorry. You're right
I mean, reports[0] contains all errors from the first contract analyzed
If you have more than one contract this will not provide correct info
Like you said I need to loop through all contracts ( all reports[] )
I will fix it today on my afternoon

Thanks a lot for the feedback !! Really appreciate

  • ❤️1
dbale-altoros commented

@madlabman made a new package on npm
whenever you can, please confirm if it is working

madlabman commented

@madlabman made a new package on npm whenever you can, please confirm if it is working

LGTM, thank you @dbale-altoros !