Skip to content
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
59 commits
Select commit Hold shift + click to select a range
4cac1b9
remove java code isImple check but move to suppression and add only c…
Jul 11, 2019
9dbeae5
resolve conflict
Jul 12, 2019
b52c5ab
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java
Jul 25, 2019
cdf50a0
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java
Jul 26, 2019
a41ffdc
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java
Jul 29, 2019
e7ea7f2
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java
Aug 2, 2019
9f7c156
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java
Aug 2, 2019
c3c3571
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java
Aug 6, 2019
06380ad
fix javadoc error
Aug 8, 2019
6d6064d
fix javadoc errors2
Aug 8, 2019
a5e48df
javadoc code snippet init
Aug 8, 2019
7187b42
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
Aug 8, 2019
340c302
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
Aug 12, 2019
5af60e2
updates based on new understand and PR feedback
Aug 13, 2019
840a047
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
Aug 13, 2019
0c7109b
refactor to isInlineCode
Aug 13, 2019
d5f2576
minor change
Aug 13, 2019
2f9cb94
resolve conflict
Aug 13, 2019
a9223ae
no checking on class-level javadoc
Aug 14, 2019
f473be0
a fix for getting correct package name
Aug 15, 2019
0746ff7
code snippet check rule
Aug 15, 2019
d5f8359
inline check rule
Aug 15, 2019
e97c88c
one java file split to two java files
Aug 15, 2019
ea9dc37
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
Aug 15, 2019
464d111
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
Aug 15, 2019
af7f4e8
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
Aug 15, 2019
5f2c597
fixes for checkstyle error and Jon's feedback
Aug 16, 2019
8cf387c
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
Aug 16, 2019
8cc72ea
more lenient naming pattern
Aug 16, 2019
f8d2228
resolve conflict
Aug 16, 2019
451f4bb
add java doc explainatino
Aug 16, 2019
513fbe8
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
Aug 18, 2019
3eea6dc
fixes for Connie's feedback
Aug 19, 2019
068e7a0
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
Aug 19, 2019
95bfe16
resolve conflict
Aug 20, 2019
ac69f96
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
Aug 21, 2019
be10e5c
updates J feedback
Aug 21, 2019
0dcaa7b
fixes and suppress errors for both checkers
Aug 22, 2019
99a5903
propose a fix for naming matching
Aug 22, 2019
59e53ba
resolve conflict
Aug 22, 2019
eb3fa99
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
Aug 22, 2019
36be65c
fixes based on V and C feedback
Aug 22, 2019
f38084e
resolve storage conflict
Aug 22, 2019
8faecd8
without Netty, redo the changes
Aug 23, 2019
88c6405
fiex errors
Aug 23, 2019
89e9a38
resolve conflict
Aug 26, 2019
5bd8728
fixes and remove comments and updates suppression
Aug 26, 2019
7997ae8
back to have suppression and remove new lines
Aug 26, 2019
ed59769
resolve conflict
Aug 26, 2019
25e36f8
add one more comment
Aug 26, 2019
d66b863
inline code
Aug 26, 2019
a5bc7fd
correct right begin end
Aug 26, 2019
6b5f663
Update eng/code-quality-reports/src/main/java/com/azure/tools/checkst…
mssfang Aug 27, 2019
f97767a
Update eng/code-quality-reports/src/main/java/com/azure/tools/checkst…
mssfang Aug 27, 2019
3e0fbd9
Update eng/code-quality-reports/src/main/java/com/azure/tools/checkst…
mssfang Aug 27, 2019
631e111
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
Aug 27, 2019
9a45b7a
not using switch but use if for single condition
Aug 27, 2019
b99e398
Merge branch 'CS-Rule-JavaDoc' of https://github.com/mssfang/azure-sd…
Aug 27, 2019
9b33a3b
storage codesnippet ID should match with begin and end code sample id…
Aug 27, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,206 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.tools.checkstyle.checks;

import com.puppycrawl.tools.checkstyle.api.DetailNode;
import com.puppycrawl.tools.checkstyle.api.JavadocTokenTypes;
import com.puppycrawl.tools.checkstyle.checks.javadoc.AbstractJavadocCheck;
import com.puppycrawl.tools.checkstyle.utils.JavadocUtil;

import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;
import java.util.regex.Pattern;

/**
* Requirement of Javadoc annotation {@literal @codesnippet} check:
* <ol>
* <li>Use {@literal {@codesnippet ...}} instead of '<code>', '<pre>', or {@literal {@code ...}) if these tags span
* multiple lines. Inline code sample are fine as-is</li>
* <ol>Naming pattern of {@literal {@codesnippet}}:
* <li>For package name, class name, they should all be lower case;
* For method name, the first letter of method name should be lower case;
* For parameters, they should only be letters. Concatenate all parameters with dash character.</li>
* <li>Jonathan will add more requirement specs later.</li>
* </ol>
* </ol>
*/
public class JavadocCodeSnippetCheck extends AbstractJavadocCheck {
// Code snippet annotation naming pattern:
// Check package name, class name, method name, parameters names' naming patterns.
private static final String CLASS_PATH_REGEX = "[a-z.]+";
private static final String METHOD_NAME_REGEX = "^[a-z][a-zA-Z]+";
private static final String PARAMETERS_REGEX = "[a-zA-Z-]+";
Comment thread
conniey marked this conversation as resolved.
Outdated

private static final String CODE_SNIPPET_ANNOTATION = "@codesnippet";
private static final String MULTIPLE_LINE_SPAN_ERROR = "Tag '%s' span in multiple lines. Use @codesnippet annotation" +
" instead of '%s' to ensure that the code block always compiles.";

// HTML tag set that need to be checked to see if there tags span on multiple lines.
private static final Set<String> CHECK_TAGS = Collections.unmodifiableSet(new HashSet<>(
Arrays.asList("pre", "code")));

@Override
public int[] getDefaultJavadocTokens() {
return getRequiredJavadocTokens();
}

@Override
public int[] getRequiredJavadocTokens() {
return new int[] {
JavadocTokenTypes.HTML_ELEMENT_START,
JavadocTokenTypes.JAVADOC_INLINE_TAG
};
}

@Override
public void visitJavadocToken(DetailNode token) {
switch (token.getType()) {
case JavadocTokenTypes.HTML_ELEMENT_START:
checkHtmlElementStart(token);
break;
case JavadocTokenTypes.JAVADOC_INLINE_TAG:
checkJavadocInlineTag(token);
checkCodeSnippetNaming(token);
default:
// Checkstyle complains if there's no default block in switch
break;
}
}

/**
* Use {@literal {@codesnippet ...}} instead of '<code>', '<pre>', or {@literal {@code ...}) if these tags span
* multiple lines. Inline code sample are fine as-is.
*
* @param htmlElementStartNode HTML_ELEMENT_START node
*/
private void checkHtmlElementStart(DetailNode htmlElementStartNode) {
final DetailNode tagNameNode = JavadocUtil.findFirstToken(htmlElementStartNode, JavadocTokenTypes.HTML_TAG_NAME);
// HTML tags are case-insensitive
final String tagName = tagNameNode.getText().toLowerCase();
if (!CHECK_TAGS.contains(tagName)) {
return;
}

final String tagNameBracket = "<" + tagName + ">";
final DetailNode htmlTagNode = htmlElementStartNode.getParent();

for (final DetailNode child : htmlTagNode.getChildren()) {
final int childType = child.getType();
if (childType == JavadocTokenTypes.NEWLINE || childType == JavadocTokenTypes.LEADING_ASTERISK) {
log(child.getLineNumber(), child.getColumnNumber(),
String.format(MULTIPLE_LINE_SPAN_ERROR, tagNameBracket, tagNameBracket));
return;
}
}
}

/**
* Check to see if the JAVADOC_INLINE_TAG node is {@literal @code} tag. If it is, check if the tag contains a new line
* or a leading asterisk, which implies the tag has spanned in multiple lines.
*
* @param inlineTagNode JAVADOC_INLINE_TAG javadoc node
*/
private void checkJavadocInlineTag(DetailNode inlineTagNode) {
final DetailNode codeLiteralNode = JavadocUtil.findFirstToken(inlineTagNode, JavadocTokenTypes.CODE_LITERAL);
if (codeLiteralNode == null) {
return;
}

final String codeLiteral = codeLiteralNode.getText();

for (final DetailNode child : inlineTagNode.getChildren()) {
final int childType = child.getType();
if (childType == JavadocTokenTypes.NEWLINE || childType == JavadocTokenTypes.LEADING_ASTERISK) {
log(child.getLineNumber(), child.getColumnNumber(),
String.format(MULTIPLE_LINE_SPAN_ERROR, codeLiteral, codeLiteral));
return;
}
}
Comment thread
mssfang marked this conversation as resolved.
}

/**
* Check to see if the code snippet name matched the naming pattern.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you show us an example of a valid naming pattern? And what conditions need to be met for this to fit the naming pattern.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

com.azure.security.keyvault.keys.cryptography.cryptographyclient.encrypt#symmetric-encrypt. More description added.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a valid naming pattern. The #symmetric-encrypt does not match the parameter types of the method here: https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/keyvault/azure-keyvault-keys/src/main/java/com/azure/security/keyvault/keys/cryptography/CryptographyClient.java#L99

My expectation that this name would actually be com.azure.security.keyvault.keys.cryptography.cryptographyclient.encrypt#encryptionalgorithm-byte-byte-byte

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should enforce that the part after the # matches the argument types of the method.

* A valid example of naming pattern could be:
* com.azure.security.keyvault.keys.cryptography.cryptographyclient.encrypt#symmetric-encrypt
*
* For more detail, please refers to this class description javadoc.
*
* @param inlineTagNode JAVADOC_INLINE_TAG javadoc node
*/
private void checkCodeSnippetNaming(DetailNode inlineTagNode) {
// Only checks on @codesnippet annotation
final DetailNode customNameNode = JavadocUtil.findFirstToken(inlineTagNode, JavadocTokenTypes.CUSTOM_NAME);
if (customNameNode == null || !CODE_SNIPPET_ANNOTATION.equals(customNameNode.getText())) {
return;
}

final DetailNode descriptionNode = JavadocUtil.findFirstToken(inlineTagNode, JavadocTokenTypes.DESCRIPTION);
if (descriptionNode == null) {
log(descriptionNode.getLineNumber(), descriptionNode.getColumnNumber(),
String.format("%s has no description.", CODE_SNIPPET_ANNOTATION));
return;
Comment thread
mssfang marked this conversation as resolved.
Outdated
}
// DESCRIPTION always has child TEXT
final DetailNode textNode = JavadocUtil.findFirstToken(descriptionNode, JavadocTokenTypes.TEXT);
final String namingPattern = textNode.getText();
// Verify naming pattern spec:
// 1. For package name, class name, they should all be lower case;
// For method name, the first letter of method name should be lower case.
// For parameters, they should only be letters. Concatenate all parameters with dash character
// 2. Jonathon will add more requirement specs later
if (!isNamingMatch(namingPattern)) {
log(textNode.getLineNumber(), textNode.getColumnNumber(),
String.format("Naming pattern mismatch. It should only contain lower case for the package path and class name for matching regular expression ''%s''" +
" and the first letter of method name must be small case for matching regular expression ''%s'', and the parameters should be all small case for matching " +
"regular expression ''%s''.", CLASS_PATH_REGEX, METHOD_NAME_REGEX, PARAMETERS_REGEX));
}
}

/**
* Find if the given name of code snippet matched the naming pattern specs.
*
* @param name code snippet name
* @return true if match, otherwise, false
*/
private boolean isNamingMatch(String name) {
final String[] str = name.split("#");

// invalid naming pattern
if (str.length == 0 || str.length > 2) {
return false;
}

final String classPath = str[0];
// corner case: empty string will also be false
if (classPath.isEmpty()) {
return false;
}

final int lastIndexOf = classPath.lastIndexOf(".");
if (lastIndexOf == -1) {
return false;
}

// full path of the class, check to see if there path only has lower case letters and dot
final String className = classPath.substring(0, lastIndexOf);
if (!Pattern.compile(CLASS_PATH_REGEX).matcher(className).matches()) {
return false;
}

// method name
final String methodName = classPath.substring(lastIndexOf + 1);
if (!Pattern.compile(METHOD_NAME_REGEX).matcher(methodName).matches()) {
return false;
}

// parameters of method function
if (str.length == 2 && !Pattern.compile(PARAMETERS_REGEX).matcher(str[1]).matches()) {
return false;
}

return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -276,33 +276,33 @@ page at http://checkstyle.sourceforge.net/config.html -->
-->
<module name="com.azure.tools.checkstyle.checks.ServiceClientChecks"/>

<!--CUSTOM CHECKS-->
<!-- CUSTOM CHECKS -->
<!--Checks for the public API that should
1) not return classes in the implementation package
2) no class of implementation package as method's parameters -->
<module name="com.azure.tools.checkstyle.checks.NoImplInPublicAPI"/>

<!--CUSTOM CHECKS-->
<!-- CUSTOM CHECKS -->
<!-- Verify the classes with annotation @ServiceClient should have following rules:
1) No public or protected constructors
2) No public static method named 'builder'
3) Since these classes are supposed to be immutable, all fields in the service client classes should be final -->
<module name="com.azure.tools.checkstyle.checks.ServiceClientInstantiationCheck"/>

<!--CUSTOM CHECKS-->
<!-- CUSTOM CHECKS -->
<!-- Checks for no external dependency exposed in the public API.
We should only return types, and accept argument types, that are from the com.azure namespace.
All other types should have suppression added if required. -->
<module name="com.azure.tools.checkstyle.checks.ExternalDependencyExposedCheck"/>

<!--CUSTOM CHECKS-->
<!-- CUSTOM CHECKS -->
<!-- The @ServiceClientBuilder class should have the following rules:
1) All service client builder should be named <ServiceName>ClientBuilder and annotated with @ServiceClientBuilder.
2) Has a method 'buildClient()' to build a synchronous client,
3) Has a method 'buildAsyncClient()' to build an asynchronous client -->
<module name="com.azure.tools.checkstyle.checks.ServiceClientBuilderCheck"/>

<!--CUSTOM CHECKS-->
<!-- CUSTOM CHECKS -->
<!-- The @ServiceInterface class should have the following rules:
1) The annotation property 'name' should be non-empty
2) The length of value of property 'name' should be less than 20 characters and without space -->
Expand All @@ -316,5 +316,16 @@ page at http://checkstyle.sourceforge.net/config.html -->
<!--CUSTOM CHECKS-->
<!-- Must use 'logger.logAndThrow' but not directly calling 'throw exception' -->
<!-- <module name="com.azure.tools.checkstyle.checks.ThrownClientLoggerCheck"/>-->

<!-- CUSTOM CHECKS -->
<!-- Requirement of Javadoc annotation @codesnippet check:
1) Use {@codesnippet ...} instead of '<code>', '<pre>', or '{@code ...}' if these tags span multiple lines.
Inline code sample are fine as-is.
2) naming pattern of @codesnippet:
a) For package name, class name, they should all be lower case.
For method name, the first letter of method name should be lower case.
For parameters, they should only be letters. Concatenate all parameters with dash character.
b) Jonathon will add more requirement specs later. -->
<module name="com.azure.tools.checkstyle.checks.JavadocCodeSnippetCheck"/>
</module>
</module>
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ public abstract class AadCredentialBuilderBase<T extends AadCredentialBuilderBas
/**
* Specifies the Azure Active Directory endpoint to acquire tokens.
* @param authorityHost the Azure Active Directory endpoint
* @return {@link <T>} itself
* @return {@code <T>} itself
*/
@SuppressWarnings("unchecked")
public T authorityHost(String authorityHost) {
Expand All @@ -24,7 +24,7 @@ public T authorityHost(String authorityHost) {
/**
* Sets the client ID of the application.
* @param clientId the client ID of the application.
* @return {@link <T>} itself
* @return {@code <T>} itself
*/
@SuppressWarnings("unchecked")
public T clientId(String clientId) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public abstract class CredentialBuilderBase<T extends CredentialBuilderBase<T>>
/**
* Specifies the max number of retries when an authentication request fails.
* @param maxRetry the number of retries
* @return {@link <T>} itself
* @return {@code <T>} itself
*/
@SuppressWarnings("unchecked")
public T maxRetry(int maxRetry) {
Expand All @@ -33,7 +33,7 @@ public T maxRetry(int maxRetry) {
/**
* Specifies a Function to calculate seconds of timeout on every retried request.
* @param retryTimeout the Function that returns a timeout in seconds given the number of retry
* @return {@link <T>} itself
* @return {@code <T>} itself
*/
@SuppressWarnings("unchecked")
public T retryTimeout(Function<Integer, Integer> retryTimeout) {
Expand All @@ -44,7 +44,7 @@ public T retryTimeout(Function<Integer, Integer> retryTimeout) {
/**
* Specifies he options for proxy configuration.
* @param proxyOptions the options for proxy configuration
* @return {@link <T>} itself
* @return {@code <T>} itself
*/
@SuppressWarnings("unchecked")
public T proxyOptions(ProxyOptions proxyOptions) {
Expand Down