Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add failOnError parameter #153

Merged
merged 8 commits into from
Jun 14, 2022
Merged

Add failOnError parameter #153

merged 8 commits into from
Jun 14, 2022

Conversation

klaraward
Copy link
Contributor

@klaraward klaraward commented May 24, 2022

Solves #59

Per discussions below naming parameter failOnError

@Stephan202
Copy link
Contributor

Is there a standard for naming this parameter? Some call it failOnWarning (which would be default true in our case)

It seems plugins use a wide range of variants; see this section for a number of examples.

(Thanks for picking this up; it'll allow us to drop this special treatment of fmt-maven-plugin 👍.)

@danielnorberg
Copy link
Contributor

danielnorberg commented May 24, 2022

Indeed there does not seem to be a strong standard around naming, but some variant of failOnXYZ seem common. E.g. failOnError, failsOnError, failOnViolation, failOnWarning, etc.

I feel a bit partial towards failOnError given true will be the default. My rationale being that we normally consider formatting errors to be hard errors that should fail the build. And so failOnError with a default of true seems consistent with the expectation that errors normally cause the build to fail. And failOnWarning with a default of true would seem inconsistent with the expectation that warnings usually do not cause builds to fail.

In comparison, the maven compiler plugin choice of failOnWarning (with a default of false) seems consistent with the expectation that warnings do not normally cause the build to fail.

@klaraward
Copy link
Contributor Author

Thanks @Stephan202 and @danielnorberg for the examples 👍
I think some failOnXYZ would be my preference. failOnNonComply is a bit bulky :)
Will change PR to use failOnWarning for now.

@klaraward klaraward changed the title Add warningOnly parameter Add failOnWarning parameter May 24, 2022
@danielnorberg
Copy link
Contributor

Do we consider the formatting violations to be "warnings" or "errors"?

I believe warnings usually do not by default fail builds whereas errors do.

If we want to keep the behavior of failing the build on formatting violations by default then I'd consider them to be "errors" and call this parameter failOnError.

@klaraward
Copy link
Contributor Author

I guess since the whole job of the check goal is to find the errors, and the output is not warnings in the process of doing something else, in contrast to the compiler plugin that compiles AND shows any warnings, that failOnError, or maybe the failOnViolation, makes a lot of sense.
failOnError is a bit easier to read and write, so lets go with tag.

@danielnorberg
Copy link
Contributor

sgtm

@klaraward klaraward changed the title Add failOnWarning parameter Add failOnError parameter Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants