[apex] Add validation of ApexDoc comments #1314
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,248 @@ | ||
| /** | ||
| * BSD-style license; for more info see http://pmd.sourceforge.net/license.html | ||
| */ | ||
|
|
||
| package net.sourceforge.pmd.lang.apex.rule.documentation; | ||
|
|
||
| import static apex.jorje.semantic.symbol.type.ModifierTypeInfos.GLOBAL; | ||
| import static apex.jorje.semantic.symbol.type.ModifierTypeInfos.OVERRIDE; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.regex.Matcher; | ||
| import java.util.regex.Pattern; | ||
|
|
||
| import org.antlr.runtime.ANTLRStringStream; | ||
| import org.antlr.runtime.Token; | ||
|
|
||
| import net.sourceforge.pmd.lang.apex.ast.ASTAnnotation; | ||
| import net.sourceforge.pmd.lang.apex.ast.ASTMethod; | ||
| import net.sourceforge.pmd.lang.apex.ast.ASTModifierNode; | ||
| import net.sourceforge.pmd.lang.apex.ast.ASTProperty; | ||
| import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; | ||
| import net.sourceforge.pmd.lang.apex.ast.ASTUserInterface; | ||
| import net.sourceforge.pmd.lang.apex.ast.ApexNode; | ||
| import net.sourceforge.pmd.lang.apex.ast.ApexRootNode; | ||
| import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; | ||
|
|
||
| import apex.jorje.data.Locations; | ||
| import apex.jorje.parser.impl.ApexLexer; | ||
| import apex.jorje.semantic.ast.member.Parameter; | ||
| import apex.jorje.semantic.ast.modifier.ModifierGroup; | ||
|
|
||
| public class ApexDocRule extends AbstractApexRule { | ||
| private static final Pattern DESCRIPTION_PATTERN = Pattern.compile("@description\\s"); | ||
| private static final Pattern RETURN_PATTERN = Pattern.compile("@return\\s"); | ||
| private static final Pattern PARAM_PATTERN = Pattern.compile("@param\\s+(\\w+)\\s"); | ||
|
|
||
| private static final String MISSING_COMMENT_MESSAGE = "Missing ApexDoc comment"; | ||
| private static final String MISSING_DESCRIPTION_MESSAGE = "Missing ApexDoc @description"; | ||
| private static final String MISSING_RETURN_MESSAGE = "Missing ApexDoc @return"; | ||
| private static final String UNEXPECTED_RETURN_MESSAGE = "Unexpected ApexDoc @return"; | ||
| private static final String MISMATCHED_PARAM_MESSAGE = "Missing or mismatched ApexDoc @param"; | ||
|
|
||
| private boolean inClass; | ||
| private boolean inTestClass; | ||
| private String source; | ||
| private List<TokenLocation> tokenLocations; | ||
|
|
||
| @Override | ||
| public Object visit(ASTUserClass node, Object data) { | ||
| if (inClass) { | ||
| super.visit(node, data); | ||
| } else { | ||
| inClass = true; | ||
| inTestClass = false; | ||
| buildTokens(node); | ||
| super.visit(node, data); | ||
| inClass = false; | ||
| } | ||
|
|
||
| handleClassOrInterface(node, data); | ||
|
|
||
| return data; | ||
| } | ||
|
|
||
|
|
||
| @Override | ||
| public Object visit(ASTUserInterface node, Object data) { | ||
| if (inClass) { | ||
| super.visit(node, data); | ||
| } else { | ||
| buildTokens(node); | ||
| super.visit(node, data); | ||
| } | ||
|
|
||
| handleClassOrInterface(node, data); | ||
|
|
||
| return data; | ||
| } | ||
|
|
||
| @Override | ||
| public Object visit(ASTAnnotation node, Object data) { | ||
| if (node.getImage().equals("IsTest")) { | ||
| inTestClass = true; | ||
| } | ||
| return data; | ||
| } | ||
|
|
||
| @Override | ||
| public Object visit(ASTMethod node, Object data) { | ||
| ApexDocComment comment = getApexDocComment(node); | ||
| if (comment == null) { | ||
| if (shouldHaveApexDocs(node)) { | ||
| addViolationWithMessage(data, node, MISSING_COMMENT_MESSAGE); | ||
| } | ||
| } else { | ||
| if (!comment.hasDescription) { | ||
| addViolationWithMessage(data, node, MISSING_DESCRIPTION_MESSAGE); | ||
| } | ||
|
|
||
| String returnType = node.getNode().getReturnTypeRef().toString(); | ||
| boolean shouldHaveReturn = !(returnType.isEmpty() || "void".equalsIgnoreCase(returnType)); | ||
| if (comment.hasReturn != shouldHaveReturn) { | ||
| if (shouldHaveReturn) { | ||
| addViolationWithMessage(data, node, MISSING_RETURN_MESSAGE); | ||
| } else { | ||
| addViolationWithMessage(data, node, UNEXPECTED_RETURN_MESSAGE); | ||
| } | ||
| } | ||
|
|
||
| ArrayList<String> params = new ArrayList<>(); | ||
| for (Parameter x : node.getNode().getMethodInfo().getParameters()) { | ||
| String value = x.getName().getValue(); | ||
| params.add(value); | ||
| } | ||
|
|
||
| if (!comment.params.equals(params)) { | ||
| addViolationWithMessage(data, node, MISMATCHED_PARAM_MESSAGE); | ||
| } | ||
| } | ||
|
|
||
| return data; | ||
| } | ||
|
|
||
| @Override | ||
| public Object visit(ASTProperty node, Object data) { | ||
| ApexDocComment comment = getApexDocComment(node); | ||
| if (comment == null) { | ||
| if (shouldHaveApexDocs(node)) { | ||
| addViolationWithMessage(data, node, MISSING_COMMENT_MESSAGE); | ||
| } | ||
| } else { | ||
| if (!comment.hasDescription) { | ||
| addViolationWithMessage(data, node, MISSING_DESCRIPTION_MESSAGE); | ||
| } | ||
| } | ||
|
|
||
| return data; | ||
| } | ||
|
|
||
| private void buildTokens(ApexRootNode<?> node) { | ||
| source = node.getSource(); | ||
| ANTLRStringStream stream = new ANTLRStringStream(source); | ||
| ApexLexer lexer = new ApexLexer(stream); | ||
|
|
||
| tokenLocations = new ArrayList<>(); | ||
| Integer startIndex = 0; | ||
| Token token = lexer.nextToken(); | ||
| Integer endIndex = lexer.getCharIndex(); | ||
| while (token.getType() != Token.EOF) { | ||
| if (token.getType() != ApexLexer.WS) { | ||
| tokenLocations.add(new TokenLocation(startIndex, token.getText())); | ||
| } | ||
| startIndex = endIndex; | ||
| token = lexer.nextToken(); | ||
| endIndex = lexer.getCharIndex(); | ||
| } | ||
| } | ||
|
|
||
| private void handleClassOrInterface(ApexNode<?> node, Object data) { | ||
| ApexDocComment comment = getApexDocComment(node); | ||
| if (comment == null) { | ||
| if (shouldHaveApexDocs(node)) { | ||
| addViolationWithMessage(data, node, MISSING_COMMENT_MESSAGE); | ||
| } | ||
| } else { | ||
| if (!comment.hasDescription) { | ||
| addViolationWithMessage(data, node, MISSING_DESCRIPTION_MESSAGE); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private boolean shouldHaveApexDocs(ApexNode<?> node) { | ||
| if (inTestClass || node.getNode().getLoc() == Locations.NONE) { | ||
| return false; | ||
| } | ||
| ASTModifierNode modifier = node.getFirstChildOfType(ASTModifierNode.class); | ||
| if (modifier != null) { | ||
| boolean isPublic = modifier.isPublic(); | ||
| ModifierGroup modifierGroup = modifier.getNode().getModifiers(); | ||
| boolean isGlobal = modifierGroup.has(GLOBAL); | ||
| boolean isOverride = modifierGroup.has(OVERRIDE); | ||
| return (isPublic || isGlobal) && !isOverride; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| private ApexDocComment getApexDocComment(ApexNode<?> node) { | ||
| String token = getApexDocToken(getApexDocIndex(node)); | ||
| if (token == null) { | ||
| return null; | ||
| } | ||
|
|
||
| boolean hasDescription = DESCRIPTION_PATTERN.matcher(token).find(); | ||
| boolean hasReturn = RETURN_PATTERN.matcher(token).find(); | ||
|
|
||
| ArrayList<String> params = new ArrayList<>(); | ||
| Matcher paramMatcher = PARAM_PATTERN.matcher(token); | ||
| while (paramMatcher.find()) { | ||
| params.add(paramMatcher.group(1)); | ||
| } | ||
|
|
||
| return new ApexDocComment(hasDescription, hasReturn, params); | ||
| } | ||
|
|
||
| private int getApexDocIndex(ApexNode<?> node) { | ||
| ASTAnnotation annotation = node.getFirstDescendantOfType(ASTAnnotation.class); | ||
| ApexNode<?> firstNode = annotation == null ? node : annotation; | ||
| int index = firstNode.getNode().getLoc().getStartIndex(); | ||
| return source.lastIndexOf('\n', index); | ||
| } | ||
|
|
||
| private String getApexDocToken(int index) { | ||
| TokenLocation last = null; | ||
| for (TokenLocation location : tokenLocations) { | ||
| if (location.index >= index) { | ||
| if (last != null && last.token.startsWith("/**")) { | ||
| return last.token; | ||
| } | ||
| return null; | ||
| } | ||
| last = location; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private class TokenLocation { | ||
| int index; | ||
| String token; | ||
|
|
||
| TokenLocation(int index, String token) { | ||
| this.index = index; | ||
| this.token = token; | ||
| } | ||
| } | ||
|
|
||
| private class ApexDocComment { | ||
|
There was a problem hiding this comment. This should be a private static class |
||
| boolean hasDescription; | ||
| boolean hasReturn; | ||
| List<String> params; | ||
|
|
||
| ApexDocComment(boolean hasDescription, boolean hasReturn, List<String> params) { | ||
| this.hasDescription = hasDescription; | ||
| this.hasReturn = hasReturn; | ||
| this.params = params; | ||
| } | ||
| } | ||
| } | ||
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -419,4 +419,15 @@ | |
| <property name="cc_block_highlighting" value="false" /> | ||
| </properties> | ||
| </rule> | ||
|
|
||
| <!-- DOCUMENTATION --> | ||
| <rule ref="category/apex/documentation.xml/ApexDoc" message="Document classes, methods, and properties that are public or global."> | ||
| <priority>3</priority> | ||
| <properties> | ||
| <!-- relevant for Code Climate output only --> | ||
| <property name="cc_categories" value="Style" /> | ||
| <property name="cc_remediation_points_multiplier" value="50" /> | ||
|
There was a problem hiding this comment. What should the multiplier be? There was a problem hiding this comment. See here for explanation of remediation points: https://github.com/codeclimate/spec/blob/master/SPEC.md#remediation-points In the implementation of our CodeClimateRenderer, we use the baseline of 50_000 (which is the estimated required time for a trivial fix) and multiply it with the property defined here: https://github.com/pmd/pmd/blob/master/pmd-core/src/main/java/net/sourceforge/pmd/renderers/CodeClimateRenderer.java#L124-L132 I see, that we use multipliers between 1 and 250. Fixing missing documentation can be time consuming and we provide here a rough guess across all types of missing doc (class documentation might take more effort than a property documentation). IMHO, 50 sounds like a reasonable value to me. |
||
| <property name="cc_block_highlighting" value="false" /> | ||
| </properties> | ||
| </rule> | ||
| </ruleset> | ||
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| /** | ||
| * BSD-style license; for more info see http://pmd.sourceforge.net/license.html | ||
| */ | ||
|
|
||
| package net.sourceforge.pmd.lang.apex.rule.documentation; | ||
|
|
||
| import net.sourceforge.pmd.testframework.SimpleAggregatorTst; | ||
|
|
||
| public class DocumentationRulesTest extends SimpleAggregatorTst { | ||
|
|
||
| private static final String RULESET = "category/apex/documentation.xml"; | ||
|
|
||
| @Override | ||
| public void setUp() { | ||
| addRule(RULESET, "ApexDoc"); | ||
| } | ||
| } |
Oops, something went wrong.
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.
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.
also, this variable is never being reset