haskell / stylish-haskell

Haskell code prettifier
Other
986 stars 150 forks source link

Consider returning 0 if we can't parse a file #341

Open michaelpj opened 3 years ago

michaelpj commented 3 years ago

At the moment, if you run stylish-haskell on a file it can't parse, then it returns 1.

However, this makes it a bit harder than necessary for tools that want to run stylish-haskell over a bunch of files and establish whether there is anything the user needs to do. For example, a pre-commit hook that runs stylish-haskell on changed files.

For such tools, "there's nothing I can do with this file" is more-or-less the same as "there is nothing to do with this file".

To put it another way, if stylish-haskell can't parse a file, there's not really anything the user can do about it. This is quite different to something like "your stylish-haskell.yaml file is malformed", where there really is a problem for the user to fix.

So maybe we should return 0 in this case.

EncodePanda commented 3 years ago

Confirmed

$ stylish-haskell -i Types.hs 

$ echo $?
0

$ echo "khjsdfkjhfsdkhjsdfkhjdfs" >> service/core-banking-gateway/src/CBGW/Types.hs 

$ stylish-haskell -i Types.hs 

$ echo $?
0
jaspervdj commented 3 years ago

Hmm, I don't think this should be the default behaviour. Returning a non-0 exit code is appropriate if there's an error and other tools seem to be doing this as well: I tested gofmt, black (python) and prettier (JavaScript).

Catching the error code in the calling program or script should be easy enough. If the caller needs more context, or wants to distinguish parse failures from other errors (e.g. bad configuration), we can returning a specific exit code for parse failures, for example 2.

If there's any need to ignore the exit code in git hooks, something like stylish-haskell args || true seems easy enough to do in bash, but maybe I'm missing something?

michaelpj commented 3 years ago

Hmmm, perhaps this would be sidestepped by asking for what we want more precisely. If, say, we had a stylish-haskell --verify command whose behaviour was "returns 1 iff running stylish-haskell on this file would result in changes", then that would be exactly what we need here.

If there's any need to ignore the exit code in git hooks, something like stylish-haskell args || true seems easy enough to do in bash, but maybe I'm missing something?

Yep we can do this, but then it would swallow errors we do want the user to address, like "your configuration file is broken".