-
-
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
[core] Better error reporting for the ruleset parser #3922
[core] Better error reporting for the ruleset parser #3922
Conversation
Generated by 🚫 Danger |
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.
Many Thanks! 🎉 This will provide much more useful error messages than ever!
I'll update your library to 3.1 and merge this.
@@ -1022,6 +1022,19 @@ | |||
</pluginRepositories> | |||
|
|||
<profiles> | |||
<profile> | |||
<!-- Adds an SLF4J implementation, useful when developing within an IDE --> |
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.
Maybe we can add this as test dependency in general?
Hm... for core, we have this already.
In java, it is missing.
Or do you need it, when you run CLI from within an IDE?
I'll keep it in, but would be interested in how you use it...
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.
I think it was to run tests like CliTest (in module pmd-java). Intellij would run them but print a warning that there is no logger implementation, and hid the messages. I'm not sure a profile is the best way, maybe just having the test dependency as you suggest would be better...
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.
@adangel I remember now where I needed this. I have a bunch of run configurations for Intellij which run the main class PMD with different arguments. Most of them use the classpath of pmd-java, so that you can launch them quicker (Intellij only needs to build pmd-java and its dependencies, not all modules). But Intellij doesn't use test dependencies when a run configuration depends on a module, so without the profile there would be no slf4j impl in this setup.
[core] Better error reporting for the ruleset parser #3922
Thanks for merging! |
They were merged on separate branches
Describe the PR
Use the library I wrote to display nice error messages when parsing the ruleset XML. This also uniformizes the error handling, which previously was a mix of IllegalArgumentExceptions and logger calls.
Sample output:
Note that as per #3816 we will stop using a
Logger
to report these errors, so that the[main] ERROR net.sourceforge.pmd.PMD
headers will disappearPreviously (notice the second error is not reported):
Related issues
Ready?
./mvnw clean verify
passes (checked automatically by github actions)