Pre-commit hooks don't error out on warnings/suggestions

[aaronmondal]

Running the pre-commit hooks with a .vale.ini containing

MinAlertLevel = suggestion

won't fail when only warning and suggestion alerts are raised.

As long as no errors are reported vale exits with code 0. Pre-commit hooks will only fail if the tool exits with a non-zero exit code. The current behavior can be checked with:

vale
echo $?

I think Vale should exit with a non-zero exit code depending on MinAlertLevel.

[jdkato]

I think Vale should exit with a non-zero exit code depending on MinAlertLevel.

This would break the ability to report multiple levels but only fail on errors (which is common use case).

The current solution here is to change the severity of rules that you want to result in a non-zero exit code:

[*]
BasedOnStyles = SomeStyle

SomeStyle.Rule = error

I get that this could be a pain if you want every rule to be an error, but I think that's generally not a good idea because sub-error rules are often that way for a reason.

[aaronmondal]

@jdkato Hmm yeah just changing the return code seems like a bad idea.

How about a --warnings-as-errors flag though? Similar to a -Werror compile flag in some compilers. The issue with Vale's current behavior is that it makes it hard to detect the usage of MinAlertLevel = suggestion in any way with tooling.

Even with that flag on, if Vale is run through pre-commit hooks, it doesn't report anything. Users would have to know that Vale is part of the hooks and manually run them in verbose mode to see warnings and suggestions.

[jdkato]

I'd say the "recommend" solution here is to use one of Vale's editor extensions, so that users can make decisions about suggestions/warnings as they're writing. It's quite possible that you want to ignore some of these and therefore wouldn't want it to block a commit.

[rpatterson]

I think having something like a ExitCodeLevel = suggestion setting would be useful in many situations. If a project needs or wants to strictly follow all of the rules in the RedHat package, then they could use this setting in their CI/CD to block PRs/MRs until any violations are resolved. On personal projects a developer may prefer to err on the side of correctness and only disable individual rules as they encounter rules they disagree with. Relying on developers' eyes to catch issues, whether in their editor or in console output, is not a good idea and kinda goes against the whole purpose of linters. The primary value of linters is blocking the developer while they're still in the context in which they introduced the sub-optimal style or format because it's much easier to address such issues then than come back to them later.

Without such a setting, developers or projects wanting strict enforcement would have to add a per-rule setting for every rule which is quite onerous given the number of rules in a package, let alone multiple packages. Those lists will also then become out of date with the packages over time. As such, they would probably have to add some external automated build process to generate Vale configurations from package rules. This is pretty ugly whereas something like a ExitCodeLevel = suggestion setting is elegant and solving the problem at the right level. I also wouldn't imagine it would be too difficult to implement given that Vale already exercises logic to make the exit code conditional on alert levels.