Build cleanup and checkstyle fixes and rules updates #2901
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <project xmlns="http://maven.apache.org/POM/4.0.0" | ||
| xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd"> | ||
| <modelVersion>4.0.0</modelVersion> | ||
|
|
||
| <groupId>com.azure</groupId> | ||
| <artifactId>sdk-build-tools</artifactId> | ||
| <name>Maven Build Tools</name> | ||
| <version>1.0.0</version> | ||
|
|
||
| <properties> | ||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
| </properties> | ||
|
|
||
| </project> | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <!DOCTYPE suppressions PUBLIC | ||
| "-//Puppy Crawl//DTD Check Configuration 1.3//EN" | ||
| "http://www.puppycrawl.com/dtds/suppressions_1_0.dtd"> | ||
|
|
||
| <suppressions> | ||
| <suppress checks="Javadoc" files="Test[s]?.*\.java"/> | ||
| <suppress checks="Javadoc" files=".*Test[s]?\.java"/> | ||
| <suppress checks="Javadoc" files=".*Test[s]?Base\.java"/> | ||
| <suppress checks="Javadoc" files=".*CoverageReporter\.java"/> | ||
| <suppress checks="Header" files=".*package-info.java"/> | ||
|
|
||
| <!-- Files licensed with alternate license in the header --> | ||
| <suppress checks="Header" files=".*AesKwBCProviderTest.java"/> | ||
| <suppress checks="Header" files=".*AesKwTest.java"/> | ||
| <suppress checks="Header" files=".*SymmetricKeyBCProviderTest.java"/> | ||
| <suppress checks="Header" files=".*SymmetricKeyTest.java"/> | ||
| <suppress checks="Header" files=".*CachingKeyResolverTest.java"/> | ||
| <suppress checks="Header" files=".*KeyVaultKeyResolverBCProviderTest.java"/> | ||
| <suppress checks="Header" files=".*KeyVaultKeyResolverDefaultProviderTest.java"/> | ||
|
|
||
| </suppressions> |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,246 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
|
|
||
| <!DOCTYPE module PUBLIC | ||
| "-//Puppy Crawl//DTD Check Configuration 1.3//EN" | ||
| "http://www.puppycrawl.com/dtds/configuration_1_3.dtd"> | ||
|
|
||
| <!-- This is a checkstyle configuration file. For descriptions of | ||
| what the following rules do, please see the checkstyle configuration | ||
| page at http://checkstyle.sourceforge.net/config.html --> | ||
|
|
||
| <module name="Checker"> | ||
|
|
||
| <module name="SuppressionFilter"> | ||
| <property name="file" value="${samedir}/checkstyle-suppressions.xml"/> | ||
| <property name="optional" value="true"/> | ||
| </module> | ||
|
|
||
| <module name="Header"> | ||
| <property name="header" value="// Copyright (c) Microsoft Corporation. All rights reserved.\n// Licensed under the MIT License."/> | ||
| <property name="fileExtensions" value="java"/> | ||
| </module> | ||
|
|
||
| <module name="FileTabCharacter"> | ||
| <!-- Checks that there are no tab characters in the file. --> | ||
| </module> | ||
|
|
||
| <module name="NewlineAtEndOfFile"> | ||
| <property name="lineSeparator" value="lf"/> | ||
| </module> | ||
|
|
||
| <module name="RegexpSingleline"> | ||
| <!-- Checks that FIXME is not used in comments. TODO is preferred. --> | ||
| <property name="format" value="((//.*)|(\*.*))FIXME"/> | ||
| <property name="message" value='TODO is preferred to FIXME. e.g. "TODO(johndoe): Refactor when v2 is released."'/> | ||
| </module> | ||
|
|
||
| <module name="RegexpSingleline"> | ||
| <!-- Checks that TODOs are named. (Actually, just that they are followed by an open paren. --> | ||
| <property name="format" value="((//.*)|(\*.*))TODO [^(]"/> | ||
| <property name="message" value='All TODOs should be named. e.g. "TODO (johndoe): Refactor when v2 is released."'/> | ||
| </module> | ||
|
|
||
| <!-- All Java AST specific tests live under TreeWalker module. --> | ||
| <module name="TreeWalker"> | ||
|
|
||
| <!-- IMPORT CHECKS --> | ||
| <!-- See http://checkstyle.sf.net/config_import.html --> | ||
| <module name="AvoidStarImport"/> | ||
| <module name="IllegalImport"/> <!-- defaults to sun.* packages --> | ||
| <module name="RedundantImport"/> | ||
| <module name="UnusedImports"/> | ||
|
|
||
| <!-- NAMING CHECKS --> | ||
| <!-- See http://checkstyle.sf.net/config_naming.html --> | ||
| <module name="ConstantName"/> | ||
| <module name="LocalFinalVariableName"/> | ||
| <module name="LocalVariableName"/> | ||
| <module name="MemberName"/> | ||
| <module name="MethodName"/> | ||
| <module name="PackageName"/> | ||
| <module name="ParameterName"/> | ||
| <module name="StaticVariableName"/> | ||
| <module name="TypeName"/> | ||
|
|
||
| <module name="TypeNameCheck"> | ||
| <!-- Validates static, final fields against the | ||
| expression "^[A-Z][a-zA-Z0-9]*$". --> | ||
| <metadata name="altname" value="TypeName"/> | ||
| <property name="severity" value="warning"/> | ||
| </module> | ||
|
|
||
| <module name="ConstantNameCheck"> | ||
| <!-- Validates non-private, static, final fields against the supplied | ||
| public/package final fields "^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$". --> | ||
| <metadata name="altname" value="ConstantName"/> | ||
| <property name="applyToPublic" value="true"/> | ||
| <property name="applyToProtected" value="true"/> | ||
| <property name="applyToPackage" value="true"/> | ||
| <property name="applyToPrivate" value="false"/> | ||
| <property name="format" value="^([A-Z][A-Z0-9]*(_[A-Z0-9]+)*|FLAG_.*)$"/> | ||
| <message key="name.invalidPattern" | ||
| value="Variable ''{0}'' should be in ALL_CAPS (if it is a constant) or be private (otherwise)."/> | ||
| <property name="severity" value="warning"/> | ||
| </module> | ||
|
|
||
| <module name="StaticVariableNameCheck"> | ||
| <!-- Validates static, non-final fields against the supplied | ||
| expression "^[a-z][a-zA-Z0-9]*_?$". --> | ||
| <metadata name="altname" value="StaticVariableName"/> | ||
| <property name="applyToPublic" value="true"/> | ||
| <property name="applyToProtected" value="true"/> | ||
| <property name="applyToPackage" value="true"/> | ||
| <property name="applyToPrivate" value="true"/> | ||
| <property name="format" value="^[a-z][a-zA-Z0-9]*_?$"/> | ||
| <property name="severity" value="warning"/> | ||
| </module> | ||
|
|
||
| <module name="MemberNameCheck"> | ||
| <!-- Validates non-static members against the supplied expression. --> | ||
| <metadata name="altname" value="MemberName"/> | ||
| <property name="applyToPublic" value="true"/> | ||
| <property name="applyToProtected" value="true"/> | ||
| <property name="applyToPackage" value="true"/> | ||
| <property name="applyToPrivate" value="true"/> | ||
| <property name="format" value="^[a-z][a-zA-Z0-9]*$"/> | ||
| <property name="severity" value="warning"/> | ||
| </module> | ||
|
|
||
| <module name="MethodNameCheck"> | ||
| <!-- Validates identifiers for method names. --> | ||
| <metadata name="altname" value="MethodName"/> | ||
| <property name="format" value="^[a-z][a-zA-Z0-9]*(_[a-zA-Z0-9]+)*$"/> | ||
| <property name="severity" value="warning"/> | ||
| </module> | ||
|
|
||
| <module name="ParameterName"> | ||
| <!-- Validates identifiers for method parameters against the | ||
| expression "^[a-z][a-zA-Z0-9]*$". --> | ||
| <property name="severity" value="warning"/> | ||
| </module> | ||
|
|
||
| <module name="LocalFinalVariableName"> | ||
| <!-- Validates identifiers for local final variables against the | ||
| expression "^[a-z][a-zA-Z0-9]*$". --> | ||
| <property name="severity" value="warning"/> | ||
| </module> | ||
|
|
||
| <module name="LocalVariableName"> | ||
| <!-- Validates identifiers for local variables against the | ||
| expression "^[a-z][a-zA-Z0-9]*$". --> | ||
| <property name="severity" value="warning"/> | ||
| </module> | ||
|
|
||
| <!-- LENGTH and CODING CHECKS --> | ||
| <module name="LeftCurly"> | ||
| <!-- Checks for placement of the left curly brace ('{'). --> | ||
| <property name="severity" value="warning"/> | ||
| </module> | ||
|
|
||
| <module name="RightCurly"> | ||
| <!-- Checks right curlies on CATCH, ELSE, and TRY blocks are on | ||
| the same line. e.g., the following example is fine: | ||
| <pre> | ||
| if { | ||
| ... | ||
| } else | ||
| </pre> | ||
| --> | ||
| <!-- This next example is not fine: | ||
| <pre> | ||
| if { | ||
| ... | ||
| } | ||
| else | ||
| </pre> | ||
| --> | ||
| <property name="option" value="same"/> | ||
| <property name="severity" value="warning"/> | ||
| </module> | ||
|
|
||
| <!-- Checks for braces around if and else blocks --> | ||
| <module name="NeedBraces"> | ||
| <property name="severity" value="warning"/> | ||
| <property name="tokens" value="LITERAL_IF, LITERAL_ELSE, LITERAL_FOR, LITERAL_WHILE, LITERAL_DO"/> | ||
| </module> | ||
|
|
||
| <module name="UpperEll"> | ||
| <!-- Checks that long constants are defined with an upper ell.--> | ||
| <property name="severity" value="error"/> | ||
| </module> | ||
|
|
||
| <module name="FallThrough"> | ||
| <!-- Warn about falling through to the next case statement. Similar to | ||
| javac -Xlint:fallthrough, but the check is suppressed if a single-line comment | ||
| on the last non-blank line preceding the fallen-into case contains 'fall through' (or | ||
| some other variants which we don't publicized to promote consistency). | ||
| --> | ||
| <property name="reliefPattern" | ||
| value="fall through|Fall through|fallthru|Fallthru|falls through|Falls through|fallthrough|Fallthrough|No break|NO break|no break|continue on"/> | ||
| <property name="severity" value="error"/> | ||
| </module> | ||
|
|
||
|
|
||
| <!-- WHITESPACE CHECKS --> | ||
| <module name="Indentation"> | ||
| <property name="basicOffset" value="4"/> | ||
| <property name="braceAdjustment" value="0"/> | ||
| <property name="caseIndent" value="4"/> | ||
| <property name="throwsIndent" value="4"/> | ||
| <property name="lineWrappingIndentation" value="4"/> | ||
| <property name="arrayInitIndent" value="4"/> | ||
| </module> | ||
|
|
||
| <!-- Checks for whitespace --> | ||
| <!-- See http://checkstyle.sf.net/config_whitespace.html --> | ||
| <module name="EmptyForIteratorPad"/> | ||
| <module name="MethodParamPad"/> | ||
| <module name="NoWhitespaceAfter"/> | ||
| <module name="NoWhitespaceBefore"/> | ||
| <module name="OperatorWrap"/> | ||
| <module name="ParenPad"/> | ||
| <module name="TypecastParenPad"/> | ||
| <module name="WhitespaceAfter"/> | ||
| <module name="WhitespaceAround"/> | ||
|
|
||
| <!-- Modifier Checks --> | ||
| <!-- See http://checkstyle.sf.net/config_modifiers.html --> | ||
| <module name="ModifierOrder"/> | ||
| <module name="RedundantModifier"/> | ||
| <module name="FileContentsHolder"/> | ||
|
|
||
| <!-- Checks for blocks. You know, those {}'s --> | ||
| <!-- See http://checkstyle.sf.net/config_blocks.html --> | ||
| <module name="AvoidNestedBlocks"/> | ||
| <module name="EmptyBlock"/> | ||
|
|
||
| <!-- Checks for common coding problems --> | ||
| <!-- See http://checkstyle.sf.net/config_coding.html --> | ||
| <module name="EmptyStatement"/> | ||
| <module name="EqualsHashCode"/> | ||
| <module name="IllegalInstantiation"/> | ||
| <module name="InnerAssignment"/> | ||
|
|
||
| <!-- Updated on 03/17/2014: --> | ||
| <!-- Added ignore. Code is generated so magic numbers are not a largish issue. --> | ||
| <!-- <module name="MagicNumber" /> --> | ||
| <module name="MissingSwitchDefault"/> | ||
| <module name="SimplifyBooleanExpression"/> | ||
| <module name="SimplifyBooleanReturn"/> | ||
|
|
||
| <!-- Checks for class design --> | ||
| <!-- See http://checkstyle.sf.net/config_design.html --> | ||
| <module name="FinalClass"/> | ||
| <module name="InterfaceIsType"/> | ||
| <module name="VisibilityModifier"> | ||
| <property name="protectedAllowed" value="true"/> | ||
| <property name="packageAllowed" value="true"/> | ||
| </module> | ||
|
|
||
|
|
||
| <!-- Miscellaneous other checks. --> | ||
| <!-- See http://checkstyle.sf.net/config_misc.html --> | ||
| <module name="ArrayTypeStyle"/> | ||
|
|
||
| </module> | ||
| </module> |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
|
|
||
| <FindBugsFilter> | ||
| </FindBugsFilter> |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <project xmlns="http://maven.apache.org/POM/4.0.0" | ||
| xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
| <parent> | ||
| <artifactId>azure-client-sdk-parent</artifactId> | ||
| <groupId>com.azure</groupId> | ||
| <version>1.0.0-SNAPSHOT</version> | ||
| <relativePath>../pom.client.xml</relativePath> | ||
|
Member
There was a problem hiding this comment. This isn't the correct path to the parent pom. One level too low. |
||
| </parent> | ||
| <modelVersion>4.0.0</modelVersion> | ||
|
|
||
| <artifactId>spotbugs-reporting</artifactId> | ||
| <version>1.0.0</version> | ||
|
|
||
| <properties> | ||
| <azure-keyvault.version>1.2.0</azure-keyvault.version> | ||
| </properties> | ||
|
|
||
| <dependencies> | ||
| <dependency> | ||
| <groupId>com.microsoft.azure</groupId> | ||
| <artifactId>azure-keyvault</artifactId> | ||
| <version>${azure-keyvault.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.microsoft.azure</groupId> | ||
| <artifactId>azure-keyvault-cryptography</artifactId> | ||
| <version>${azure-keyvault.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.microsoft.azure</groupId> | ||
| <artifactId>azure-keyvault-core</artifactId> | ||
| <version>${azure-keyvault.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.microsoft.azure</groupId> | ||
| <artifactId>azure-keyvault-extensions</artifactId> | ||
| <version>${azure-keyvault.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.microsoft.azure</groupId> | ||
| <artifactId>azure-keyvault-complete</artifactId> | ||
| <version>${azure-keyvault.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.microsoft.azure</groupId> | ||
| <artifactId>azure-keyvault-webkey</artifactId> | ||
| <version>${azure-keyvault.version}</version> | ||
| </dependency> | ||
| </dependencies> | ||
|
Member
There was a problem hiding this comment. Do we need to specify the dependencies AND the source below?
Member
Author
There was a problem hiding this comment. Yes, |
||
|
|
||
| <build> | ||
| <plugins> | ||
| <plugin> | ||
| <groupId>org.codehaus.mojo</groupId> | ||
| <artifactId>build-helper-maven-plugin</artifactId> | ||
| <executions> | ||
| <execution> | ||
| <id>add-source</id> | ||
| <phase>generate-sources</phase> | ||
| <goals> | ||
| <goal>add-source</goal> | ||
| </goals> | ||
| <configuration> | ||
| <sources> | ||
| <source>..\keyvault\data-plane\azure-keyvault\src\main\java</source> | ||
|
Member
There was a problem hiding this comment. Does this really require that we list all the source directories manually here to run the checks?
Member
Author
There was a problem hiding this comment. yeah, spotbugs doesn't offer aggregated reports. This way requires manually specifying sources.
Member
Author
There was a problem hiding this comment. This aggregates all the spotbugs in one report.
Member
There was a problem hiding this comment. That is going to be a pain to maintain. We should be on the look out for potential ways to configure or generate this list so we don't have to add each project individually.
Member
Author
There was a problem hiding this comment. yes, agreed, |
||
| <source>..\keyvault\data-plane\azure-keyvault-cryptography\src\main\java</source> | ||
| <source>..\keyvault\data-plane\azure-keyvault-core\src\main\java</source> | ||
| <source>..\keyvault\data-plane\azure-keyvault-webkey\src\main\java</source> | ||
| <source>..\keyvault\data-plane\azure-keyvault-extensions\src\main\java</source> | ||
| <source>..\keyvault\data-plane\azure-keyvault-complete\src\main\java</source> | ||
| </sources> | ||
| </configuration> | ||
| </execution> | ||
| </executions> | ||
| </plugin> | ||
| </plugins> | ||
| </build> | ||
|
|
||
|
|
||
| </project> | ||
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
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.
In other repos we are starting to try and switch to using a folder called "eng" to hold files specific engineering system tools and such. I'd suggest calling the folder "eng" instead of "buid-tools".
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.
Actually it seems as though this is actually project so I'd suggest you still put it under an "eng" folder like "eng\build-tool" assuming you want to call it that. It would probably be best to name it after what it is trying to do.
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.
My gut feeling is that we should keep this as /build-tools - it's a pretty standard approach in Java to expose build-related tools from the root level. E.g. AWS and GCP. I'm not against having it under a sub-directory, but 1) I wonder what else we might put under there (e.g. in the future presumably we will have a top-level azure-sdk-bom directory, etc, and that wouldn't go under eng) and 2) if 'eng' is really a good, descriptive name for visitors to the repo.
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.
@weshaggard Can you give your thoughts here please? Thanks.
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 don't have a strong preference here but my goal is to drive consistency across the language repos as much as possible while still allowing for the language conventions where it makes sense. One of my goals is to reduce the clutter in the root of the repo which is why I like having a top level directory that we store anything that is infrastructure/engineering system related into. In general I feel most contributors to the repo generally care more about the source code and less about the engineering system so keeping it scoped mostly in a directory helps eliminate that clutter for a lot of people. For your azure-sdk-bom directory I think that is more of a "source code" type of change, especially if we plan to ship it, so having it in its own directory would make sense. As for the name "eng" we could debate that but it is a convention we have used on my previous teams and it is short name that people who care about what is in there generally learn what it means.
That said I'll leave this call up to you @JonathanGiles if you feel that having the build-tools directory makes more sense in the scope of Java developers please continue with that name, but if you feel this is something that most contributors to the repo won't care about then I'd like to use a common convention and put it under a "eng" sub-dir.
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'd also add that if there going to be multiple of these tools (e.g. spotcheck, checkstyle, etc) then I personally think having them named after what they are checking and under a sub-directory makes the most sense.
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'm ok with using eng. I'm work with Vinay on directory naming
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.
Directories are renamed.
build-tools is -> eng/code-quality-reports
eng/spotbugs-reporting -> eng/spotbugs-aggregate-report.