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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 9 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
8 changes: 7 additions & 1 deletion src/main/java/com/google/api/generator/engine/ast/ClassDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@

@AutoValue
public abstract class ClassDefinition implements AstNode {
// Required for outer classes.
@Nullable
xiaozhenliu-gg5 marked this conversation as resolved.
Show resolved Hide resolved
public abstract ImmutableList<CommentStatement> fileHeader();
// Required.
public abstract ScopeNode scope();
// Required.
Expand Down Expand Up @@ -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 Expand Up @@ -122,8 +127,9 @@ public ClassDefinition build() {

ClassDefinition classDef = autoBuild();

// Only nested classes can forego having a package.
// Only nested classes can forego having a package and file header.
if (!classDef.isNested()) {
Preconditions.checkNotNull(classDef.fileHeader(), "Outer classes must have a file header.");
xiaozhenliu-gg5 marked this conversation as resolved.
Show resolved Hide resolved
Preconditions.checkNotNull(
classDef.packageString(), "Outer classes must have a package name defined");
Preconditions.checkState(!classDef.isStatic(), "Outer classes cannot be static");
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,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
1 change: 1 addition & 0 deletions src/main/java/com/google/api/generator/gapic/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package(default_visibility = ["//visibility:public"])
filegroup(
name = "gapic_files",
srcs = glob(["*.java"]) + [
"//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
51 changes: 51 additions & 0 deletions src/main/java/com/google/api/generator/gapic/composer/FileHeader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// 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.CommentStatement;
import com.google.api.generator.engine.ast.LineComment;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

public class FileHeader {
xiaozhenliu-gg5 marked this conversation as resolved.
Show resolved Hide resolved
public static final List<CommentStatement> APACHE_LICENSE = createApacheLicense();
xiaozhenliu-gg5 marked this conversation as resolved.
Show resolved Hide resolved

xiaozhenliu-gg5 marked this conversation as resolved.
Show resolved Hide resolved
private static List<CommentStatement> createApacheLicense() {
String[] fileHeadeStrings = {
"Copyright 2020 Google LLC",
xiaozhenliu-gg5 marked this conversation as resolved.
Show resolved Hide resolved
"",
"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."
};

List<CommentStatement> apacheFileHeader = new ArrayList<>();
Arrays.stream(fileHeadeStrings)
.forEach(
s -> {
apacheFileHeader.add(CommentStatement.withComment(LineComment.withComment(s)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Clients (example)currently have this in a BlockComment, so let's use that 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.

fix for BlockComment is here: #168
It cannot be wrapped in the current blockComment because the formatting will be messed up, more details are in #168 PR description. Thanks

});
return apacheFileHeader;
}
}
1 change: 1 addition & 0 deletions ...java/com/google/api/generator/gapic/composer/GrpcServiceCallableFactoryClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ public GapicClass generate(Service service, Map<String, Message> ignore) {

ClassDefinition classDef =
ClassDefinition.builder()
.setFileHeader(FileHeader.APACHE_LICENSE)
xiaozhenliu-gg5 marked this conversation as resolved.
Show resolved Hide resolved
.setPackageString(pakkage)
.setAnnotations(createClassAnnotations(types))
.setImplementsTypes(createClassImplements(types))
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/com/google/api/generator/gapic/composer/GrpcServiceStubClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ public GapicClass generate(Service service, Map<String, Message> ignore) {

ClassDefinition classDef =
ClassDefinition.builder()
.setFileHeader(FileHeader.APACHE_LICENSE)
.setPackageString(pakkage)
.setAnnotations(createClassAnnotations())
.setScope(ScopeNode.PUBLIC)
Expand Down Expand Up @@ -232,7 +233,8 @@ private static Statement createMethodDescriptorVariableDecl(
return ExprStatement.withExpr(
AssignmentExpr.builder()
.setVariableExpr(
methodDescriptorVarExpr.toBuilder()
methodDescriptorVarExpr
.toBuilder()
.setIsDecl(true)
.setScope(ScopeNode.PRIVATE)
.setIsStatic(true)
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/api/generator/gapic/composer/MockServiceClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ public GapicClass generate(Service service, Map<String, Message> ignore) {

ClassDefinition classDef =
ClassDefinition.builder()
.setFileHeader(FileHeader.APACHE_LICENSE)
.setPackageString(pakkage)
.setAnnotations(createClassAnnotations(types))
.setScope(ScopeNode.PUBLIC)
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/api/generator/gapic/composer/MockServiceImplClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ public GapicClass generate(Service service, Map<String, Message> ignore) {

ClassDefinition classDef =
ClassDefinition.builder()
.setFileHeader(FileHeader.APACHE_LICENSE)
.setPackageString(pakkage)
.setAnnotations(createClassAnnotations())
.setScope(ScopeNode.PUBLIC)
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ public GapicClass generate(Service service, Map<String, Message> messageTypes) {

ClassDefinition classDef =
ClassDefinition.builder()
.setFileHeader(FileHeader.APACHE_LICENSE)
.setPackageString(pakkage)
.setAnnotations(createClassAnnotations(types))
.setImplementsTypes(createClassImplements(types))
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/api/generator/gapic/composer/ServiceClientTestClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ public GapicClass generate(Service service, Map<String, Message> ignore) {

ClassDefinition classDef =
ClassDefinition.builder()
.setFileHeader(FileHeader.APACHE_LICENSE)
.setPackageString(pakkage)
.setAnnotations(createClassAnnotations())
.setScope(ScopeNode.PUBLIC)
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/api/generator/gapic/composer/ServiceSettingsClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ public GapicClass generate(Service service, Map<String, Message> ignore) {

ClassDefinition classDef =
ClassDefinition.builder()
.setFileHeader(FileHeader.APACHE_LICENSE)
.setPackageString(pakkage)
.setAnnotations(createClassAnnotations())
.setScope(ScopeNode.PUBLIC)
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/api/generator/gapic/composer/ServiceStubClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ public GapicClass generate(Service service, Map<String, Message> messageTypes) {

ClassDefinition classDef =
ClassDefinition.builder()
.setFileHeader(FileHeader.APACHE_LICENSE)
.setPackageString(pakkage)
.setAnnotations(createClassAnnotations(types))
.setIsAbstract(true)
Expand Down
32 changes: 32 additions & 0 deletions src/test/java/com/google/api/generator/engine/ast/ClassDefinitionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 @@ -64,6 +65,7 @@ public void validClassDefinition_nestedBasic() {
@Test
public void validClassDefinition_withAnnotationsExtendsAndImplements() {
ClassDefinition.builder()
.setFileHeader(createFileHeader())
xiaozhenliu-gg5 marked this conversation as resolved.
Show resolved Hide resolved
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setScope(ScopeNode.PUBLIC)
Expand Down Expand Up @@ -108,6 +110,7 @@ public void validClassDefinition_statementsAndMethods() {
List<MethodDefinition> methods = Arrays.asList(method, method);

ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setIsFinal(true)
Expand All @@ -124,6 +127,7 @@ public void invalidClassDefinition_implementsNullType() {
IllegalStateException.class,
() -> {
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setScope(ScopeNode.PUBLIC)
Expand All @@ -132,12 +136,26 @@ public void invalidClassDefinition_implementsNullType() {
});
}

@Test
public void invalidClassDefinition_outerClassMissingFileHeader() {
xiaozhenliu-gg5 marked this conversation as resolved.
Show resolved Hide resolved
assertThrows(
NullPointerException.class,
() -> {
ClassDefinition.builder()
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setScope(ScopeNode.PUBLIC)
.build();
});
}

@Test
public void invalidClassDefinition_outerClassMissingPackage() {
assertThrows(
NullPointerException.class,
() -> {
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setName("LibraryServiceStub")
.setScope(ScopeNode.PUBLIC)
.build();
Expand All @@ -150,6 +168,7 @@ public void invalidClassDefinition_outerClassStatic() {
IllegalStateException.class,
() -> {
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setIsStatic(true)
Expand All @@ -164,6 +183,7 @@ public void invalidClassDefinition_outerClassPrivate() {
IllegalStateException.class,
() -> {
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setScope(ScopeNode.PRIVATE)
Expand All @@ -177,6 +197,7 @@ public void invalidClassDefinition_extendsPrimitiveType() {
IllegalStateException.class,
() -> {
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setScope(ScopeNode.PUBLIC)
Expand All @@ -191,6 +212,7 @@ public void invalidClassDefinition_extendsNullType() {
IllegalStateException.class,
() -> {
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setScope(ScopeNode.PUBLIC)
Expand All @@ -205,6 +227,7 @@ public void invalidClassDefinition_abstractFinal() {
IllegalStateException.class,
() -> {
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setIsAbstract(true)
Expand All @@ -221,6 +244,7 @@ public void invalidClassDefinition_implementsPrimtiveType() {
IllegalStateException.class,
() -> {
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setScope(ScopeNode.PUBLIC)
Expand All @@ -236,6 +260,7 @@ public void invalidClassDefinition_extendsImplementsSameType() {
IllegalStateException.class,
() -> {
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setScope(ScopeNode.PUBLIC)
Expand Down Expand Up @@ -265,6 +290,7 @@ public void invalidClassDefinition_assignmentWithUnscopedVariableExprStatement()
IllegalStateException.class,
() -> {
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setScope(ScopeNode.PUBLIC)
Expand All @@ -287,6 +313,7 @@ public void invalidClassDefinition_unscopedVariableExprStatement() {
IllegalStateException.class,
() -> {
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setScope(ScopeNode.PUBLIC)
Expand All @@ -302,6 +329,7 @@ public void invalidClassDefinition_badStatementType() {
IllegalStateException.class,
() -> {
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setScope(ScopeNode.PUBLIC)
Expand Down Expand Up @@ -347,4 +375,8 @@ private static ForStatement createForStatement() {
.setBody(Arrays.asList(ExprStatement.withExpr(createAssignmentExpr())))
.build();
}
// Create a simple line 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 •
edited
Loading

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(LineComment.withComment("Apache License")));
}
}
1 change: 0 additions & 1 deletion src/test/java/com/google/api/generator/engine/writer/ImportWriterVisitorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,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
17 changes: 14 additions & 3 deletions src/test/java/com/google/api/generator/engine/writer/JavaWriterVisitorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1502,6 +1502,7 @@ public void writeMethodDefinition_templatedReturnTypeAndArguments() {
public void writeClassDefinition_basic() {
ClassDefinition classDef =
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setScope(ScopeNode.PUBLIC)
Expand All @@ -1511,7 +1512,8 @@ public void writeClassDefinition_basic() {
assertEquals(
writerVisitor.write(),
String.format(
createLines(3),
createLines(4),
"// Apache License\n\n",
"package com.google.example.library.v1.stub;\n",
"\n",
"public class LibraryServiceStub {}\n"));
Expand All @@ -1521,6 +1523,7 @@ public void writeClassDefinition_basic() {
public void writeClassDefinition_withAnnotationsExtendsAndImplements() {
ClassDefinition classDef =
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setName("LibraryServiceStub")
.setScope(ScopeNode.PUBLIC)
Expand All @@ -1540,7 +1543,8 @@ public void writeClassDefinition_withAnnotationsExtendsAndImplements() {
assertEquals(
writerVisitor.write(),
String.format(
createLines(5),
createLines(6),
"// Apache License\n\n",
"package com.google.example.library.v1.stub;\n",
"\n",
"@Deprecated\n",
Expand Down Expand Up @@ -1628,6 +1632,7 @@ public void writeClassDefinition_commentsStatementsAndMethods() {

ClassDefinition classDef =
ClassDefinition.builder()
.setFileHeader(createFileHeader())
.setPackageString("com.google.example.library.v1.stub")
.setHeaderCommentStatements(
Arrays.asList(
Expand All @@ -1643,7 +1648,8 @@ public void writeClassDefinition_commentsStatementsAndMethods() {
classDef.accept(writerVisitor);
String expected =
String.format(
createLines(36),
createLines(37),
"// Apache License\n\n",
"package com.google.example.library.v1.stub;\n",
"\n",
"import com.google.api.generator.engine.ast.AssignmentExpr;\n",
Expand Down Expand Up @@ -1788,4 +1794,9 @@ private static String createSampleCode() {
tryCatch.accept(writerVisitor);
return writerVisitor.write();
}

// Create a simple line comment to stand for the Apache License header.
private static List<CommentStatement> createFileHeader() {
return Arrays.asList(CommentStatement.withComment(LineComment.withComment("Apache License")));
}
}
Loading