-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[cli] Add exit code for processing errors #4991
base: master
Are you sure you want to change the base?
Conversation
Generated by 🚫 Danger |
[skip ci]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR
I think we should also consider CPD's --skip-lexical-errors
with this PR. It seems to me we should deprecate --skip-lexical-errors
in favor of the new flag.
Currently CPD lexes all files and once it is done, if there was an error it recovered from, it just gives up if --skip-lexical-errors
is not set. I think this is not good behavior, as we already recovered from all errors at this point. In this case, I think we should continue execution and just make sure the exit code reflects whether there were errors or not (also respecting the flags --fail-on-violation
and --fail-on-error
). This would align PMD and CPD closer.
Another thing is I think we should stop using the phrase "processing error" in our doc and especially in the new switch --fail-on-processing-error
. What a "processing error" is is vague and although we use it sometimes in our doc we don't explain it anywhere. It is likely our users have no idea how a "processing error" is different from an "error".
FWIW what we call today a "processing error" is just an error we recovered from gracefully, and which didn't prevent us from outputting a report. There is no difference in nature from other errors, just in how we handled it. That's why I think this phrase it weird and cryptic. There are two kinds of errors in PMD/CPD:
- Errors which were unexpected and we did not recover from. These cause exit code 1, and when they happen, then the report is either non-existent (we failed before reporting) or broken (we failed during reporting).
- Errors from which we recovered gracefully ("processing errors"). They did not prevent the creation of a valid report, and these errors are actually part of the rendered report. Under your proposal these would cause exit code 5.
I think it would be better if we dropped the phrase "processing error" and just referred to these as "errors", or "recovered errors" if that's not clear from context.
So I would like to have the new flag named --[no-]fail-on-error
without the processing
.
This comment was marked as resolved.
This comment was marked as resolved.
pmd-core/src/main/java/net/sourceforge/pmd/AbstractConfiguration.java
Outdated
Show resolved
Hide resolved
pmd-core/src/main/java/net/sourceforge/pmd/AbstractConfiguration.java
Outdated
Show resolved
Hide resolved
- release notes: API Changes - fix javadoc since tags - improve messages in ant task for deprecated skipLexicalErrors
Describe the PR
This adds a new exit code 5 when processing errors occurred (both in PMD and CPD).
This helps in avoiding that such errors are unnoticed.
I think, it's better to fail in such cases than to ignore processing errors.
Not sure, whether an extra exit code 5 really beneficial or needed or we should reuse exit code 4 (which is violations only). The problem I see with exit code 5, you only know that one or more processing errors occurred, but you don't know whether there were any violations. We could of course add another exit code 6 (Only processing errors but no violations). I don't know, whether this is really needed.
The main goal I had is: I want to fail the build in case of processing errors. The result of a failing build is usually the same (regardless of the failure cause): I need to look at the build, logs, changes, to figure out the problem and solve it. The exit code gives only a little bit more information.
Interestingly, this feature was already present in the ant task. It is requested for gradle (gradle/gradle#28672). And the feature is also available in maven-pmd-plugin, but disabled by default. There is a request to fail the build by default in the presence of processing errors (https://issues.apache.org/jira/browse/MPMD-383).
Related issues
Fixes [cli] Consider processing errors in exit status #2827
Related in broader context: [core] Error handling in PMD 7 #3761
Ready?
./mvnw clean verify
passes (checked automatically by github actions)