[core] Pmd7 test cleanups#2807
Merged
Merged
Conversation
Generated by 🚫 Danger |
The supported API is now just the junit integration
Member
Author
|
@adangel I'm going to remove the API-breaking changes from RuleTst (so that we can decide on that later) and merge this PR this week |
adangel
reviewed
Oct 22, 2020
| /** | ||
| * @deprecated Use {@link #ParametricRuleViolation(Rule, String, Node, String)} | ||
| */ | ||
| @Deprecated |
Member
There was a problem hiding this comment.
Deprecated or DeprecatedUntil700?
If deprecated, we need to merge this back to master -> API change...
| * arguments to embed in the rule violation message | ||
| */ | ||
| void addViolation(RuleContext ruleContext, Rule rule, @Nullable Node node, String message, Object[] args); | ||
| @Deprecated |
Member
There was a problem hiding this comment.
Probably DeprecatedUntil700 as well?
| */ | ||
| public class ParserOptions { | ||
| protected String suppressMarker; | ||
| private String suppressMarker = PMD.SUPPRESS_MARKER; |
Member
There was a problem hiding this comment.
I think, we should deprecate this field on master...
adangel
added a commit
that referenced
this pull request
Oct 22, 2020
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the PR
Cleanup tests by introducing new utilities, like
BaseParsingHelper#executeRule(Rule,String) -> Report. This removes a lot of usage sites of RuleContext, and prepares further refactoring of this object.This also makes many places in the violation reporting codebase more defensive in requiring their parameters be non-null. Many tests build dysfunctional violations by passing a null node or null file name or whatnot, so these needed to be cleaned up.
ParametricRuleViolation is changed to not require a rule context, but instead a file name. This removes a lot of places where we create a rule context just to set the filename or the language version. The RuleViolationFactory interface is also changed, to only require an implementation of createViolation and suppressOrNull.
The longer-term idea, is to expunge RuleContext of all state, by moving getSourceCodeFile and getLanguageVersion onto the Node interface. Given that, RuleContext doesn't need to store this state, and so has no purpose anymore. I think, we can repurpose it to be the API for rules to report violations. It would basically have the overloads of
addViolationthat AbstractRule has for now, but clearer. In the end, the idea is for rules to have type-safe access to a RuleContext and report violations directly on it instead of usingAbstractRule#addViolation(Object, ...).Related issues
Ready?
./mvnw clean verifypasses (checked automatically by travis)