jednano / eclint

Validate or fix code that doesn't adhere to EditorConfig settings or infer settings from existing code.
MIT License
307 stars 28 forks source link

eclint does’t exit with non-zero status when checking multiple files and only some has errors #29

Closed jirutka closed 9 years ago

jirutka commented 9 years ago

If any errors are reported, the Node process will exit with a status code of 1…

This doesn’t work when multiple files are globbed and errors are reported only in some of these files.

$ eclint check '**/*'; echo $?
test-me.adoc: line 2: trailing whitespace found
0

$ node --version
v0.12.7
jednano commented 9 years ago

I just published v0.2.7. Tell me if that fixes it.

jirutka commented 9 years ago

Nope, it’s worse, now it exits with 0 even when checking just one file.

btw It’s better to let it test before releasing new version.

jednano commented 9 years ago

I can always revert, which I just did. Also, I only had a 10-minute window to look into this and that has just expired. If you can submit a breaking test I can look into it more. As for now, all tests are passing, including on Travis.

jirutka commented 9 years ago

As for now, all tests are passing, including on Travis.

Yeah, because you don’t have any test for the cli module (or I just can’t find it). So it’s also not so easy for me to add test for this particular behaviour, because there’s no scaffolding prepared.

Well, you can unpublish the package, but that should be used very rarely! It’s not a “revert” in true sense of the word…

Even if a package version is unpublished, that specific name and version combination can never be reused. In order to publish the package again, a new version number must be used.

jednano commented 9 years ago

You're right. There are no tests for the CLI module, which is why it's not a quick fix.

And yeah, I'm not going to unpublish the package. What I did was much easier/faster and gave you something you could test.

jirutka commented 9 years ago

I’m (of course) able to install eclint from repository…

jednano commented 9 years ago

Thanks for pointing me in the right direction. Fixed in v1.0.0, https://github.com/jedmao/eclint/commit/b75a2e0c458eceb97223f4f8bba69fe32c33a803.

jirutka commented 9 years ago

You’ve “solved” the bug by deprecating node 0.10.x? You must be kidding me… I know that supporting outdated versions sucks, but 0.10.x is still officially maintained and 0.12.0 has been released only 5 months ago. I personally don’t use binary linux distros with repositories that resemble museums, but most people do, so I can’t use a tool that don’t work “out of box” for most people.

Okay, I’ll find another tool for checking editorconfig rules.

jednano commented 9 years ago

Feel free to submit a fix for Node 0.10 if you have a solution; otherwise, there's editorconfig-tools with 14 open issues or echint, which looks like potentially a good alternative.

jednano commented 9 years ago

Fixed in https://github.com/jedmao/eclint/commit/f3a87fb382d53ae4f2f8eb694556bda5d208d850

jednano commented 9 years ago

@jirutka BTW – Next time someone gives you a free sandwich, try not to complain that it doesn't have any cheese in it. You might win more friends that way.

jirutka commented 9 years ago

The problem with exit status is still not fixed.

jednano commented 9 years ago

You're absolutely right. Fixed in v1.1.1: https://github.com/jedmao/eclint/commit/20633c34c264761bd009505c5a559ca584eff6bc

$ eclint check "src/**"; echo $?
src\lib\plugin.ts: line 7: invalid indentation: found a leading space, expected: tab
1