Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ggj] feat: support file header (apache license) for outer class definition #157

Merged
merged 30 commits into from
Aug 19, 2020
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
4d48254
add file header comment
xiaozhenliu-gg5 Aug 6, 2020
712a2b9
gapic unit tests pass
xiaozhenliu-gg5 Aug 6, 2020
aecb241
fix bazel rules
xiaozhenliu-gg5 Aug 6, 2020
35cf94c
merge master
xiaozhenliu-gg5 Aug 6, 2020
d8d9cb1
clean up
xiaozhenliu-gg5 Aug 6, 2020
4044ee4
format code
xiaozhenliu-gg5 Aug 6, 2020
85e87a4
file header class
xiaozhenliu-gg5 Aug 6, 2020
44c0cf5
simplify create header
xiaozhenliu-gg5 Aug 6, 2020
23813f8
format
xiaozhenliu-gg5 Aug 6, 2020
18f6267
new line after file header
xiaozhenliu-gg5 Aug 6, 2020
7b0eea2
merge
xiaozhenliu-gg5 Aug 7, 2020
c0d3be7
file header is optional for class
xiaozhenliu-gg5 Aug 7, 2020
88d71cf
add composer test
xiaozhenliu-gg5 Aug 7, 2020
45dbadb
Merge branch 'master' of github.com:googleapis/gapic-generator-java
xiaozhenliu-gg5 Aug 8, 2020
0f59ef8
multiple line comment
xiaozhenliu-gg5 Aug 8, 2020
2707ea1
Merge branch 'fix-block-comment' of github.com:googleapis/gapic-gener…
xiaozhenliu-gg5 Aug 8, 2020
0f34aec
multiple line comment for header
xiaozhenliu-gg5 Aug 8, 2020
6364167
rename apache license
xiaozhenliu-gg5 Aug 10, 2020
590c7c6
move file header addition to gapic class
xiaozhenliu-gg5 Aug 11, 2020
a356da9
format
xiaozhenliu-gg5 Aug 11, 2020
50c1fb1
convert change in Composer
xiaozhenliu-gg5 Aug 11, 2020
31c0468
add edge unit test
xiaozhenliu-gg5 Aug 12, 2020
7ef4b4a
move header addition to composer class
xiaozhenliu-gg5 Aug 12, 2020
ede98cb
Merge branch 'fix-block-comment' of github.com:googleapis/gapic-gener…
xiaozhenliu-gg5 Aug 12, 2020
d59e1a3
format
xiaozhenliu-gg5 Aug 12, 2020
7a19396
feedback
xiaozhenliu-gg5 Aug 14, 2020
7879471
merge master
xiaozhenliu-gg5 Aug 16, 2020
5eccd75
Merge branch 'master' of github.com:googleapis/gapic-generator-java i…
xiaozhenliu-gg5 Aug 17, 2020
43f24c0
feedback
xiaozhenliu-gg5 Aug 18, 2020
e051381
clean up
xiaozhenliu-gg5 Aug 19, 2020
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
Expand Up @@ -14,7 +14,6 @@

package com.google.api.generator.engine.ast;

import com.google.api.generator.engine.escaper.MetacharEscaper;
import com.google.auto.value.AutoValue;

@AutoValue
Expand All @@ -38,14 +37,6 @@ public static BlockComment withComment(String comment) {
public abstract static class Builder {
public abstract Builder setComment(String comment);

// Private accessor.
abstract String comment();

public abstract BlockComment autoBuild();

public BlockComment build() {
setComment(MetacharEscaper.escaper(comment()));
return autoBuild();
}
public abstract BlockComment build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@

@AutoValue
public abstract class ClassDefinition implements AstNode {
// Optional.
public abstract ImmutableList<CommentStatement> fileHeader();
// Required.
public abstract ScopeNode scope();
// Required.
Expand Down Expand Up @@ -66,6 +68,7 @@ public void accept(AstNodeVisitor visitor) {

public static Builder builder() {
return new AutoValue_ClassDefinition.Builder()
.setFileHeader(Collections.emptyList())
.setHeaderCommentStatements(Collections.emptyList())
.setIsNested(false)
.setIsFinal(false)
Expand All @@ -80,6 +83,8 @@ public static Builder builder() {

@AutoValue.Builder
public abstract static class Builder {
public abstract Builder setFileHeader(List<CommentStatement> fileHeader);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to error-out in build() if this is a nested class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, added one condition check in build to confirm nested classes do not have file headers.


public abstract Builder setHeaderCommentStatements(
List<CommentStatement> headerCommentStatements);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,15 @@ public class JavaWriterVisitor implements AstNodeVisitor {

private static final String COLON = ":";
private static final String COMMA = ",";
private static final String BLOCK_COMMENT_START = "/**";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please separate the changes in #168 from this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hold this PR until #168 is merged.

private static final String BLOCK_COMMENT_START = "/*";
private static final String BLOCK_COMMENT_END = "*/";
private static final String DOT = ".";
private static final String ESCAPED_QUOTE = "\"";
private static final String EQUALS = "=";
private static final String LEFT_ANGLE = "<";
private static final String LEFT_BRACE = "{";
private static final String LEFT_PAREN = "(";
private static final String JAVADOC_COMMENT_START = "/**";
private static final String QUESTION_MARK = "?";
private static final String RIGHT_ANGLE = ">";
private static final String RIGHT_BRACE = "}";
Expand Down Expand Up @@ -487,17 +488,21 @@ public void visit(LineComment lineComment) {
}

public void visit(BlockComment blockComment) {
// Split comments by new line and embrace the comment block with `/** */`.
String sourceComment = blockComment.comment();
String formattedSource =
JavaFormatter.format(
String.format("%s %s %s", BLOCK_COMMENT_START, sourceComment, BLOCK_COMMENT_END));
buffer.append(formattedSource);
// Split comments by new line and embrace the comment block with `/* */`.
StringBuilder sourceComment = new StringBuilder();
sourceComment.append(BLOCK_COMMENT_START).append(NEWLINE);
Arrays.stream(blockComment.comment().split("\\r?\\n"))
.forEach(
comment -> {
sourceComment.append(String.format("%s %s%s", ASTERISK, comment, NEWLINE));
});
sourceComment.append(BLOCK_COMMENT_END);
buffer.append(JavaFormatter.format(sourceComment.toString()));
}

public void visit(JavaDocComment javaDocComment) {
StringBuilder sourceComment = new StringBuilder();
sourceComment.append(BLOCK_COMMENT_START).append(NEWLINE);
sourceComment.append(JAVADOC_COMMENT_START).append(NEWLINE);
Arrays.stream(javaDocComment.comment().split("\\r?\\n"))
.forEach(
comment -> {
Expand Down Expand Up @@ -624,6 +629,8 @@ public void visit(MethodDefinition methodDefinition) {
@Override
public void visit(ClassDefinition classDefinition) {
if (!classDefinition.isNested()) {
statements(classDefinition.fileHeader().stream().collect(Collectors.toList()));
newline();
importWriterVisitor.initialize(
classDefinition.packageString(), classDefinition.classIdentifier().name());
buffer.append(String.format("package %s;", classDefinition.packageString()));
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/api/generator/gapic/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package(default_visibility = ["//visibility:public"])
filegroup(
name = "gapic_files",
srcs = glob(["*.java"]) + [
"//src/test/java/com/google/api/generator/gapic/composer:composer_files",
"//src/main/java/com/google/api/generator/gapic/composer:composer_files",
"//src/main/java/com/google/api/generator/gapic/model:model_files",
"//src/main/java/com/google/api/generator/gapic/protoparser:protoparser_files",
],
miraleung marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Copyright 2020 Google LLC
//
// Licensed 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
//
// http://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.

package com.google.api.generator.gapic.composer;

import com.google.api.generator.engine.ast.BlockComment;
import com.google.api.generator.engine.ast.CommentStatement;
import java.util.Arrays;
import java.util.List;

public class ApacheLicenseComposer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this again, maybe call this class "ApacheLicense" instead, since it doesn't do any composing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since it's not a composer, how about moving it to utils?

private static final String fileHeadeString =
"Copyright 2020 Google LLC\n\n"
+ "Licensed under the Apache License, Version 2.0 (the \"License\");\n"
+ "you may not use this file except in compliance with the License.\n"
+ "You may obtain a copy of the License at\n\n"
+ " https://www.apache.org/licenses/LICENSE-2.0\n\n"
+ "Unless required by applicable law or agreed to in writing, software\n"
+ "distributed under the License is distributed on an \"AS IS\" BASIS,\n"
+ "WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.\n"
+ "See the License for the specific language governing permissions and\n"
+ "limitations under the License.";

public static final List<CommentStatement> APACHE_LICENSE =
xiaozhenliu-gg5 marked this conversation as resolved.
Show resolved Hide resolved
Arrays.asList(CommentStatement.withComment(BlockComment.withComment(fileHeadeString)));;
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.google.api.generator.gapic.model.Message;
import com.google.api.generator.gapic.model.ResourceName;
import com.google.api.generator.gapic.model.Service;
import com.google.common.annotations.VisibleForTesting;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -85,7 +86,8 @@ public static List<GapicClass> generateMocksAndTestClasses(
}

/** ====================== STUB CLASSES ==================== */
private static GapicClass generateStubServiceSettings(Service service) {
@VisibleForTesting
protected static GapicClass generateStubServiceSettings(Service service) {
return generateGenericClass(
Kind.STUB, String.format("%sStubSettings", service.name()), service);
}
Expand All @@ -100,6 +102,7 @@ private static GapicClass generateGenericClass(Kind kind, String name, Service s

ClassDefinition classDef =
ClassDefinition.builder()
.setFileHeader(ApacheLicenseComposer.APACHE_LICENSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

We currently generate many classes, and this method is a placeholder that will be going away. Can you think of ways to restructure this such that we can add a license to a class and test that in isolation? How would we incorporate that into the existing classes?

Copy link
Contributor Author

@xiaozhenliu-gg5 xiaozhenliu-gg5 Aug 11, 2020

Choose a reason for hiding this comment

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

sure, good point. Since we are creating different GapicClasses in the composers, I think a central place to add a file header is GapicClass class. Call GapicClass.create().addApacheLicnese() for adding license to existing class. 3 reasons are in consideration:

  1. ClassDefinition is another alternative place to have this helper, but in the end, it's gapic class that needs/wraps it, so putting the helper in gapic class might make more sense.
  2. The way to incorporate header to the existing class is GapicClass.create(kind, classDef).addApacheLicense();
  3. it can be independently tested in GapicClassTest without altering composer files.

Alternatives:

  1. tried to have a private helper addApacheLicense(GapicClass) in Composer class, but in that way we will call it by clazzes.add(**Composer.instance().generate().addApacheLicense()) I think adding file header is more suitable in a class level, instead of composer level.
  2. add the helper to ApacheLicense as a util, but ApacheLicense.addLicenseToGapicClass() method should makes more sense to directly be called from the class side GapicClass.addApacheLicense()
    Let me know your thoughts @miraleung Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

GapicClass has no extra logic beyond holding data, so let's keep it that way. Let's add a helper method in Composer.java that adds license headers to each GapicClass instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the Composer class and add ComposerTest to test file header addition in isolation.

.setPackageString(pakkage)
.setName(name)
.setScope(ScopeNode.PUBLIC)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,8 @@ private static Statement createMethodDescriptorVariableDecl(
return ExprStatement.withExpr(
AssignmentExpr.builder()
.setVariableExpr(
methodDescriptorVarExpr.toBuilder()
methodDescriptorVarExpr
.toBuilder()
.setIsDecl(true)
.setScope(ScopeNode.PRIVATE)
.setIsStatic(true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
public class ClassDefinitionTest {

@Test
public void validClassDefinition_basicWithComments() {
public void validClassDefinition_basicWithCommentsAndHeader() {
LineComment lineComment = LineComment.withComment("AUTO-GENERATED DOCUMENTATION AND CLASS");
JavaDocComment javaDocComment =
JavaDocComment.builder()
Expand All @@ -32,6 +32,7 @@ public void validClassDefinition_basicWithComments() {
"This class is for advanced usage and reflects the underlying API directly.")
.build();
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setHeaderCommentStatements(
Arrays.asList(
CommentStatement.withComment(lineComment),
Expand Down Expand Up @@ -347,4 +348,8 @@ private static ForStatement createForStatement() {
.setBody(Arrays.asList(ExprStatement.withExpr(createAssignmentExpr())))
.build();
}
// Create a simple block comment to stand for the Apache License header.
private static List<CommentStatement> createFileHeader() {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is just used once, do we need a helper method for it? Would prefer to just add the logic to the method where it's used.

Copy link
Contributor Author

@xiaozhenliu-gg5 xiaozhenliu-gg5 Aug 18, 2020

Choose a reason for hiding this comment

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

Updated with adding a unit test (nestClassWithFileHeader) which also uses this. So I guess we can keep it. But this similar helper is only called once in JavaWriterVisitorTest, so removed it there. Thanks

return Arrays.asList(CommentStatement.withComment(BlockComment.withComment("Apache License")));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ public void writeNewObjectExprImports_withArgs() {
.setArguments(Arrays.asList(fileExpr))
.build();
newObjectExpr.accept(writerVisitor);
System.out.println(writerVisitor.write());
assertEquals(
writerVisitor.write(),
String.format(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ public void writeBlockCommentStatement_basic() {
String content = "this is a test comment";
BlockComment blockComment = BlockComment.builder().setComment(content).build();
CommentStatement commentStatement = CommentStatement.withComment(blockComment);
String expected = "/** this is a test comment */\n";
String expected = "/*\n" + "* this is a test comment\n" + "*/\n";
commentStatement.accept(writerVisitor);
assertEquals(writerVisitor.write(), expected);
}
Expand Down Expand Up @@ -400,11 +400,10 @@ public void writeJavaDocCommentStatement_allComponents() {
}

@Test
public void writeBlockComment_specialChar() {
String content = "Testing special characters: \b\t\n\r\"`'?/\\,.[]{}|-_!@#$%^()";
public void writeBlockComment_multipleLine() {
String content = "Apache License \nThis is a test file header";
BlockComment blockComment = BlockComment.builder().setComment(content).build();
String expected =
"/** Testing special characters: \\b\\t\\n\\r\"`'?/\\\\,.[]{}|-_!@#$%^() */\n";
String expected = "/*\n" + "* Apache License\n" + "* This is a test file header\n" + "*/\n";
blockComment.accept(writerVisitor);
assertEquals(writerVisitor.write(), expected);
}
Expand Down Expand Up @@ -1499,9 +1498,10 @@ public void writeMethodDefinition_templatedReturnTypeAndArguments() {
}

@Test
public void writeClassDefinition_basic() {
public void writeClassDefinition_basicWithFileHeader() {
ClassDefinition classDef =
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setScope(ScopeNode.PUBLIC)
Expand All @@ -1511,7 +1511,10 @@ public void writeClassDefinition_basic() {
assertEquals(
writerVisitor.write(),
String.format(
createLines(3),
createLines(6),
"/*\n",
" * Apache License\n",
" */\n\n",
"package com.google.example.library.v1.stub;\n",
"\n",
"public class LibraryServiceStub {}\n"));
Expand Down Expand Up @@ -1788,4 +1791,9 @@ private static String createSampleCode() {
tryCatch.accept(writerVisitor);
return writerVisitor.write();
}

// Create a simple block comment to stand for the Apache License header.
private static List<CommentStatement> createFileHeader() {
return Arrays.asList(CommentStatement.withComment(BlockComment.withComment("Apache License")));
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package(default_visibility = ["//visibility:public"])

TESTS = [
"ComposerTest",
"GrpcServiceCallableFactoryClassComposerTest",
"GrpcServiceStubClassComposerTest",
"MockServiceClassComposerTest",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// Copyright 2020 Google LLC
//
// Licensed 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
//
// http://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.

package com.google.api.generator.gapic.composer;

import static junit.framework.Assert.assertEquals;

import com.google.api.generator.engine.writer.JavaWriterVisitor;
import com.google.api.generator.gapic.model.Message;
import com.google.api.generator.gapic.model.ResourceName;
import com.google.api.generator.gapic.model.Service;
import com.google.api.generator.gapic.protoparser.Parser;
import com.google.protobuf.Descriptors.FileDescriptor;
import com.google.protobuf.Descriptors.ServiceDescriptor;
import com.google.showcase.v1beta1.EchoOuterClass;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.junit.Before;
import org.junit.Test;

public class ComposerTest {
private ServiceDescriptor echoService;
private FileDescriptor echoFileDescriptor;

@Before
public void setUp() {
echoFileDescriptor = EchoOuterClass.getDescriptor();
echoService = echoFileDescriptor.getServices().get(0);
assertEquals(echoService.getName(), "Echo");
}

@Test
public void generateStubServiceSettings() {
Map<String, Message> messageTypes = Parser.parseMessages(echoFileDescriptor);
Map<String, ResourceName> resourceNames = Parser.parseResourceNames(echoFileDescriptor);
Set<ResourceName> outputResourceNames = new HashSet<>();

List<Service> services =
Parser.parseService(echoFileDescriptor, messageTypes, resourceNames, outputResourceNames);
Service echoProtoService = services.get(0);
JavaWriterVisitor visitor = new JavaWriterVisitor();
Composer.generateStubServiceSettings(echoProtoService).classDefinition().accept(visitor);
xiaozhenliu-gg5 marked this conversation as resolved.
Show resolved Hide resolved
System.out.println(visitor.write());
System.out.println(EXPECTED_CLASS_STRING);

assertEquals(visitor.write(), EXPECTED_CLASS_STRING);
}

private static final String EXPECTED_CLASS_STRING =
"/*\n"
+ " * Copyright 2020 Google LLC\n"
+ " *\n"
+ " * Licensed under the Apache License, Version 2.0 (the \"License\");\n"
+ " * you may not use this file except in compliance with the License.\n"
+ " * You may obtain a copy of the License at\n"
+ " *\n"
+ " * https://www.apache.org/licenses/LICENSE-2.0\n"
+ " *\n"
+ " * Unless required by applicable law or agreed to in writing, software\n"
+ " * distributed under the License is distributed on an \"AS IS\" BASIS,\n"
+ " * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.\n"
+ " * See the License for the specific language governing permissions and\n"
+ " * limitations under the License.\n"
+ " */\n\n"
+ "package com.google.showcase.v1beta1.stub;\n\n"
+ "public class EchoStubSettings {}\n";
}