This is an automated email from the ASF dual-hosted git repository. jhyde pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/calcite.git
commit 54e3faf0618c25a63b1c40c0ec3855ce0b842127 Author: Julian Hyde <[email protected]> AuthorDate: Wed Jun 14 12:15:35 2023 -0700 Add various lint checks (The errors detected by these rules were fixed in the preceding commit.) Add a lint check '// not followed by space'. Add a lint rule that ':' in 'for' must be surrounded by spaces. Add a lint rule disallowing '?' without ':' on an assignment line. It Converts v = b ? branch1 : branch2; to v = b ? branch1 : branch2; Apply lint rules to all text files, not just Java files. Detect tabs and trailing spaces. Add 'lint:skip' directive. Exempt LintTest from lint rules, because it contains strings that are designed to trigger lint rules. Treat the lint test string as a Java file. In Puffin, add method Line.matcher(regex). --- build.gradle.kts | 5 +- .../main/java/org/apache/calcite/util/Puffin.java | 17 +++-- .../java/org/apache/calcite/test/LintTest.java | 72 ++++++++++++++++++++-- .../java/org/apache/calcite/util/TestUnsafe.java | 20 +++++- 4 files changed, 99 insertions(+), 15 deletions(-) diff --git a/build.gradle.kts b/build.gradle.kts index 4b78f37ed7..90ce625516 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -603,8 +603,9 @@ allprojects { replaceRegex("Method parameter list should not end in whitespace or newline", "(?<!;)\\s+\\) \\{", ") {") replaceRegex("Method argument list should not end in whitespace or newline", "(?<!;)\\s+(\\)[);,])", "$1") replaceRegex("Method argument list should not end in whitespace or newline", "(?<!;)(\\s+)(\\)+)[.]", "$2$1.") - replaceRegex("Long assignment should be broken after '='", "^([^/*\"\\n]*) = ([^@\\n]*)\\(\n( *)", "$1 =\n$3$2(") - replaceRegex("Long assignment should be broken after '='", "^([^/*\"\\n]*) = ([^@\\n]*)\\((.*,)\n( *)", "$1 =\n$4$2($3 ") + replaceRegex("Long assignment should be broken after '=' (#1)", "^([^/*\"\\n]*) = ([^@\\n]*)\\(\n( *)", "$1 =\n$3$2(") + replaceRegex("Long assignment should be broken after '=' (#2)", "^([^/*\"\\n]*) = ([^@\\n]*)\\((.*,)\n( *)", "$1 =\n$4$2($3 ") + replaceRegex("Long assignment should be broken after '=' (#3)", "^([^/*\"\\n]*) = ([^@\\n\"?]*[ ][?][ ][^@\\n:]*)\n( *)", "$1 =\n$3$2\n$3 ") replaceRegex("trailing keyword: implements", "^([^*]*) (implements)\\n( *)", "$1\n$3$2 ") // Assume developer copy-pasted the link, and updated text only, so the url is old, and we replace it with the proper one replaceRegex(">[CALCITE-...] link styles: 1", "<a(?:(?!CALCITE-)[^>])++CALCITE-\\d+[^>]++>\\s*+\\[?(CALCITE-\\d+)\\]?", "<a href=\"https://issues.apache.org/jira/browse/\$1\";>[\$1]") diff --git a/core/src/main/java/org/apache/calcite/util/Puffin.java b/core/src/main/java/org/apache/calcite/util/Puffin.java index 8b5d6e286c..cf52a92d57 100644 --- a/core/src/main/java/org/apache/calcite/util/Puffin.java +++ b/core/src/main/java/org/apache/calcite/util/Puffin.java @@ -35,6 +35,7 @@ import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; import java.util.function.Supplier; +import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Stream; @@ -171,6 +172,8 @@ public class Puffin { /** Returns whether the current line matches {@code regex}. */ boolean matches(String regex); + Matcher matcher(String regex); + /** Returns the text of the current line. */ String line(); @@ -248,14 +251,14 @@ public class Puffin { return fnr; } - @Override public String filename() { - return source().toString(); - } - @Override public boolean isLast() { return last; } + @Override public String filename() { + return source().toString(); + } + @Override public Source source() { return source; } @@ -272,8 +275,12 @@ public class Puffin { return line.endsWith(suffix); } + @Override public Matcher matcher(String regex) { + return pattern(regex).matcher(line); + } + @Override public boolean matches(String regex) { - return pattern(regex).matcher(line).matches(); + return matcher(regex).matches(); } @Override public String line() { diff --git a/core/src/test/java/org/apache/calcite/test/LintTest.java b/core/src/test/java/org/apache/calcite/test/LintTest.java index bb1119d298..591ccb0f61 100644 --- a/core/src/test/java/org/apache/calcite/test/LintTest.java +++ b/core/src/test/java/org/apache/calcite/test/LintTest.java @@ -55,6 +55,26 @@ class LintTest { .add(line -> line.fnr() == 1, line -> line.globalState().fileCount++) + // Skip directive + .add(line -> line.matches(".* lint:skip ([0-9]+).*"), + line -> { + final Matcher matcher = line.matcher(".* lint:skip ([0-9]+).*"); + if (matcher.matches()) { + int n = Integer.parseInt(matcher.group(1)); + line.state().skipToLine = line.fnr() + n; + } + }) + + // Trailing space + .add(line -> line.endsWith(" "), + line -> line.state().message("Trailing space", line)) + + // Tab + .add(line -> line.contains("\t") + && !line.filename().endsWith(".txt") + && !skipping(line), + line -> line.state().message("Tab", line)) + // Comment without space .add(line -> line.matches(".* //[^ ].*") && !line.source().fileOpt() @@ -65,13 +85,19 @@ class LintTest { && !line.contains("//CHECKSTYLE"), line -> line.state().message("'//' must be followed by ' '", line)) + // In 'for (int i : list)', colon must be surrounded by space. + .add(line -> line.matches("^ *for \\(.*:.*") + && !line.matches(".*[^ ][ ][:][ ][^ ].*") + && isJava(line.filename()), + line -> line.state().message("':' must be surrounded by ' '", line)) + // Javadoc does not require '</p>', so we do not allow '</p>' .add(line -> line.state().inJavadoc() && line.contains("</p>"), line -> line.state().message("no '</p>'", line)) // No "**/" - .add(line -> line.contains("**/") + .add(line -> line.contains(" **/") && line.state().inJavadoc(), line -> line.state().message("no '**/'; use '*/'", @@ -105,7 +131,8 @@ class LintTest { && line.state().blockquoteCount == 0 && line.contains("* ") && line.fnr() - 1 == line.state().starLine - && line.matches("^ *\\* [^<@].*"), + && line.matches("^ *\\* [^<@].*") + && isJava(line.filename()), line -> line.state().message("missing '<p>'", line)) // The first "@param" of a javadoc block must be preceded by a blank @@ -141,6 +168,22 @@ class LintTest { .build(); } + /** Returns whether we are currently in a region where lint rules should not + * be applied. */ + private static boolean skipping(Puffin.Line<GlobalState, FileState> line) { + return line.state().skipToLine >= 0 + && line.fnr() < line.state().skipToLine; + } + + /** Returns whether we are in a file that contains Java code. */ + private static boolean isJava(String filename) { + return filename.endsWith(".java") + || filename.endsWith(".jj") + || filename.endsWith(".fmpp") + || filename.endsWith(".ftl") + || filename.equals("GuavaCharSource{memory}"); // for testing + } + @Test void testProgramWorks() { final String code = "class MyClass {\n" + " /** Paragraph.\n" @@ -155,7 +198,17 @@ class LintTest { + " * @see java.lang.String (should be preceded by blank line)\n" + " **/\n" + " String x = \"ok because it's not in javadoc:</p>\";\n" - + " //comment without space\n" + + " for (Map.Entry<String, Integer> e: entries) {\n" + + " //comment without space\n" + + " }\n" + + " for (int i :tooFewSpacesAfter) {\n" + + " }\n" + + " for (int i : tooManySpacesBefore) {\n" + + " }\n" + + " for (int i : tooManySpacesAfter) {\n" + + " }\n" + + " for (int i : justRight) {\n" + + " }\n" + "}\n"; final String expectedMessages = "[" + "GuavaCharSource{memory}:4:" @@ -173,7 +226,15 @@ class LintTest { + "GuavaCharSource{memory}:12:" + "no '**/'; use '*/'\n" + "GuavaCharSource{memory}:14:" + + "':' must be surrounded by ' '\n" + + "GuavaCharSource{memory}:15:" + "'//' must be followed by ' '\n" + + "GuavaCharSource{memory}:17:" + + "':' must be surrounded by ' '\n" + + "GuavaCharSource{memory}:19:" + + "':' must be surrounded by ' '\n" + + "GuavaCharSource{memory}:21:" + + "':' must be surrounded by ' '\n" + ""; final Puffin.Program<GlobalState> program = makeProgram(); final StringWriter sw = new StringWriter(); @@ -191,11 +252,11 @@ class LintTest { assumeTrue(TestUnsafe.haveGit(), "Invalid git environment"); final Puffin.Program<GlobalState> program = makeProgram(); - final List<File> javaFiles = TestUnsafe.getJavaFiles(); + final List<File> files = TestUnsafe.getTextFiles(); final GlobalState g; try (PrintWriter pw = Util.printWriter(System.out)) { - g = program.execute(javaFiles.parallelStream().map(Sources::of), pw); + g = program.execute(files.parallelStream().map(Sources::of), pw); } g.messages.forEach(System.out::println); @@ -338,6 +399,7 @@ class LintTest { /** Internal state of the lint rules, per file. */ private static class FileState { final GlobalState global; + int skipToLine; int starLine; int atLine; int javadocStartLine; diff --git a/core/src/test/java/org/apache/calcite/util/TestUnsafe.java b/core/src/test/java/org/apache/calcite/util/TestUnsafe.java index a2ae509f7b..e9e24118c0 100644 --- a/core/src/test/java/org/apache/calcite/util/TestUnsafe.java +++ b/core/src/test/java/org/apache/calcite/util/TestUnsafe.java @@ -148,14 +148,28 @@ public abstract class TestUnsafe { return true; } - /** Returns a list of Java files in git under a given directory. + /** Returns a list of Java files in git. */ + public static List<File> getJavaFiles() { + return getGitFiles("*.java"); + } + + /** Returns a list of text files in git. */ + public static List<File> getTextFiles() { + return getGitFiles("*.bat", "*.cmd", "*.csv", "*.fmpp", "*.ftl", + "*.iq", "*.java", "*.json", "*.jj", "*.kt", "*.kts", "*.md", + "*.properties", "*.sh", "*.sql", "*.txt", "*.xml", "*.yaml", + "*.yml"); + } + + /** Returns a list of files in git matching a given pattern or patterns. * * <p>Assumes running Linux or macOS, and that git is available. */ - public static List<File> getJavaFiles() { + public static List<File> getGitFiles(String... patterns) { String s; try { final List<String> argumentList = - ImmutableList.of("git", "ls-files", "*.java"); + ImmutableList.<String>builder().add("git").add("ls-files") + .add(patterns).build(); final File base = TestUtil.getBaseDir(TestUnsafe.class); try { final StringWriter sw = new StringWriter();
