You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@avro.apache.org by GitBox <gi...@apache.org> on 2021/12/11 14:25:01 UTC
[GitHub] [avro] RyanSkraba commented on a change in pull request #1377: AVRO-3239: IDL parsing silently ignores dangling documentation comments
RyanSkraba commented on a change in pull request #1377:
URL: https://github.com/apache/avro/pull/1377#discussion_r767157719
##########
File path: lang/java/compiler/src/test/java/org/apache/avro/compiler/idl/DocCommentHelperTest.java
##########
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Some portions of this file were modeled after the example Java 1.5
Review comment:
Unnecessary header I think!
##########
File path: lang/java/compiler/src/main/java/org/apache/avro/compiler/idl/DocCommentHelper.java
##########
@@ -0,0 +1,155 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Some portions of this file were modeled after the example Java 1.5
Review comment:
It's probably safe to remove the header lines from here to the end -- I don't think any of your source code here was modeled after JavaCC code from 2005!
##########
File path: lang/java/compiler/src/main/java/org/apache/avro/compiler/idl/DocCommentHelper.java
##########
@@ -0,0 +1,155 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Some portions of this file were modeled after the example Java 1.5
+ * parser included with JavaCC. The following license applies to those
+ * portions:
+ *
+ * Copyright (c) 2006, Sun Microsystems, Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * * Redistributions of source code must retain the above copyright notice,
+ * this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ * * Neither the name of the Sun Microsystems, Inc. nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+package org.apache.avro.compiler.idl;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Utility class with {@code ThreadLocal} fields that allow the generated
+ * classes {@link Idl} and {@link IdlTokenManager} to exchange documentation
+ * comments without forcing explicit parsing of documentation comments.
+ *
+ * The reason this works is that all calls to this class happen within a call to
+ * the method {@link Idl#CompilationUnit()} (either directly or indirectly).
+ */
+public class DocCommentHelper {
+ /**
+ * Pattern to match the common whitespace indents in a multi-line String.
+ * Doesn't match a single-line String, fully matches any multi-line String.
+ *
+ * To use: match on a {@link String#trim() trimmed} String, and then replace all
+ * newlines followed by the group "indent" with a newline.
+ */
+ private static final Pattern WS_INDENT = Pattern.compile("(?U).*\\R(?<indent>\\h*).*(?:\\R\\k<indent>.*)*");
+ /**
+ * Pattern to match the whitespace indents plus common stars (1 or 2) in a
+ * multi-line String. If a String fully matches, replace all occurrences of a
+ * newline followed by whitespace and then the group "stars" with a newline.
+ *
+ * Note: partial matches are invalid.
+ */
+ private static final Pattern STAR_INDENT = Pattern.compile("(?U)(?<stars>\\*{1,2}).*(?:\\R\\h*\\k<stars>.*)*");
+
+ private static final ThreadLocal<DocComment> DOC = new ThreadLocal<>();
+ private static final ThreadLocal<List<String>> WARNINGS = ThreadLocal.withInitial(ArrayList::new);
+
+ /**
+ * Return all warnings that were encountered while parsing, once. Subsequent
+ * calls before parsing again will return an empty list.
+ */
+ static List<String> getAndClearWarnings() {
+ List<String> warnings = WARNINGS.get();
+ WARNINGS.remove();
+ return warnings;
+ }
+
+ static void setDoc(Token token) {
+ DocComment newDocComment = new DocComment(token);
+ DocComment oldDocComment = DOC.get();
+ if (oldDocComment != null) {
+ WARNINGS.get().add(String.format(
+ "Found documentation comment at line %d, column %d. Ignoring previous one at line %d, column %d: \"%s\"\n"
+ + "A common cause is to use documentation comments ( /** ... */ ) instead of multiline comments ( /* ... */ ).",
Review comment:
Here and below, I think it's more grammatical to say `A common cause is using documentation comments...`
It might even be clearer to say `Did you mean to use a multiline comment ( /* ... */` ) instead?`
What do you think?
##########
File path: lang/java/compiler/src/test/java/org/apache/avro/compiler/idl/DocCommentHelperTest.java
##########
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Some portions of this file were modeled after the example Java 1.5
+ * parser included with JavaCC. The following license applies to those
+ * portions:
+ *
+ * Copyright (c) 2006, Sun Microsystems, Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * * Redistributions of source code must retain the above copyright notice,
+ * this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ * * Neither the name of the Sun Microsystems, Inc. nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+package org.apache.avro.compiler.idl;
+
+import junit.framework.TestCase;
+
+import static java.util.Collections.emptyList;
+import static java.util.Collections.singletonList;
+
+public class DocCommentHelperTest extends TestCase {
+ public void testNoWarnings() {
+ DocCommentHelper.getAndClearWarnings(); // Clear warnings
+
+ DocCommentHelper.setDoc(token(1, 1, "This is a token."));
+ assertEquals(DocCommentHelper.getDoc(), "This is a token.");
+ DocCommentHelper.clearDoc(); // Should be a no-op. If not, it adds a warning.
+ assertEquals("There should be no warnings", emptyList(), DocCommentHelper.getAndClearWarnings());
+ }
+
+ /**
+ * Create a doc comment token. Does not include the initial '/**'.
+ *
+ * @param line the line where the comment starts
+ * @param column the column where the comment starts (the position
+ * of the '/**')
+ * @param tokenWithoutSuffix the comment content (without the trailing
+ * '<span>*</span>/')
+ * @return a mock token
+ */
+ private Token token(int line, int column, String tokenWithoutSuffix) {
+ final Token token = new Token();
+ token.image = tokenWithoutSuffix + "*/";
+ token.beginLine = line;
+ token.beginColumn = column + 3;
+ return token;
+ }
+
+ public void testWarningAfterSecondDoc() {
+ DocCommentHelper.getAndClearWarnings(); // Clear warnings
+
+ DocCommentHelper.setDoc(token(3, 2, "This is the first token."));
+ DocCommentHelper.setDoc(token(5, 4, "This is the second token."));
+ assertEquals(DocCommentHelper.getDoc(), "This is the second token.");
+ assertEquals("There should be no warnings", singletonList(
+ "Found documentation comment at line 5, column 4. Ignoring previous one at line 3, column 2: \"This is the first token.\"\n"
+ + "A common cause is to use documentation comments ( /** ... */ ) instead of multiline comments ( /* ... */ )."),
+ DocCommentHelper.getAndClearWarnings());
+ }
+
+ public void testWarningAfterUnusedDoc() {
+ DocCommentHelper.getAndClearWarnings(); // Clear warnings
+
+ DocCommentHelper.setDoc(token(3, 2, "This is a token."));
+ DocCommentHelper.clearDoc();
+ assertNull(DocCommentHelper.getDoc());
+ assertEquals("There should be no warnings",
Review comment:
```suggestion
assertEquals("There should be a warning",
```
##########
File path: lang/java/compiler/src/test/java/org/apache/avro/compiler/idl/DocCommentHelperTest.java
##########
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Some portions of this file were modeled after the example Java 1.5
+ * parser included with JavaCC. The following license applies to those
+ * portions:
+ *
+ * Copyright (c) 2006, Sun Microsystems, Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * * Redistributions of source code must retain the above copyright notice,
+ * this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ * * Neither the name of the Sun Microsystems, Inc. nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+package org.apache.avro.compiler.idl;
+
+import junit.framework.TestCase;
+
+import static java.util.Collections.emptyList;
+import static java.util.Collections.singletonList;
+
+public class DocCommentHelperTest extends TestCase {
+ public void testNoWarnings() {
+ DocCommentHelper.getAndClearWarnings(); // Clear warnings
+
+ DocCommentHelper.setDoc(token(1, 1, "This is a token."));
+ assertEquals(DocCommentHelper.getDoc(), "This is a token.");
+ DocCommentHelper.clearDoc(); // Should be a no-op. If not, it adds a warning.
+ assertEquals("There should be no warnings", emptyList(), DocCommentHelper.getAndClearWarnings());
+ }
+
+ /**
+ * Create a doc comment token. Does not include the initial '/**'.
+ *
+ * @param line the line where the comment starts
+ * @param column the column where the comment starts (the position
+ * of the '/**')
+ * @param tokenWithoutSuffix the comment content (without the trailing
+ * '<span>*</span>/')
+ * @return a mock token
+ */
+ private Token token(int line, int column, String tokenWithoutSuffix) {
+ final Token token = new Token();
+ token.image = tokenWithoutSuffix + "*/";
+ token.beginLine = line;
+ token.beginColumn = column + 3;
+ return token;
+ }
+
+ public void testWarningAfterSecondDoc() {
+ DocCommentHelper.getAndClearWarnings(); // Clear warnings
+
+ DocCommentHelper.setDoc(token(3, 2, "This is the first token."));
+ DocCommentHelper.setDoc(token(5, 4, "This is the second token."));
+ assertEquals(DocCommentHelper.getDoc(), "This is the second token.");
+ assertEquals("There should be no warnings", singletonList(
Review comment:
```suggestion
assertEquals("There should be a warning", singletonList(
```
##########
File path: .github/workflows/test-lang-java.yml
##########
@@ -103,7 +103,7 @@ jobs:
- name: Install Java Avro for Interop Test
working-directory: .
- run: mvn -B install -DskipTests
+ run: mvn -B clean install -DskipTests
Review comment:
Is this here just to be explicit or did you find some issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org