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

xiaozhenliu-gg5 · 2020-08-06T19:11:33Z

No description provided.

src/main/java/com/google/api/generator/gapic/BUILD.bazel

src/main/java/com/google/api/generator/gapic/composer/FileHeader.java

src/main/java/com/google/api/generator/engine/ast/ClassDefinition.java

src/main/java/com/google/api/generator/gapic/composer/FileHeader.java

miraleung · 2020-08-07T01:20:29Z

src/main/java/com/google/api/generator/gapic/composer/FileHeader.java

+    Arrays.stream(fileHeadeStrings)
+        .forEach(
+            s -> {
+              apacheFileHeader.add(CommentStatement.withComment(LineComment.withComment(s)));


Clients (example)currently have this in a BlockComment, so let's use that instead.

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

src/main/java/com/google/api/generator/gapic/composer/FileHeader.java

...in/java/com/google/api/generator/gapic/composer/GrpcServiceCallableFactoryClassComposer.java

src/test/java/com/google/api/generator/engine/ast/ClassDefinitionTest.java

src/test/java/com/google/api/generator/gapic/composer/GrpcServiceStubClassComposerTest.java

…ator-java into file-header

xiaozhenliu-gg5 · 2020-08-09T00:00:10Z

Left some questions in the unresolved comments @miraleung Summarized them here for clarity:

altered BlockComment class for supporting the file header composer.
all file header addition to *Composer classes are removed since it's optional, but add the file header to Composer.java itself and test it in new ComposerTest.java.

src/main/java/com/google/api/generator/gapic/composer/ApacheLicenseComposer.java

miraleung · 2020-08-10T21:21:49Z

src/main/java/com/google/api/generator/gapic/composer/ApacheLicenseComposer.java

+import java.util.Arrays;
+import java.util.List;
+
+public class ApacheLicenseComposer {


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

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

src/test/java/com/google/api/generator/gapic/composer/ComposerTest.java

miraleung · 2020-08-10T21:27:07Z

src/main/java/com/google/api/generator/gapic/composer/Composer.java

@@ -100,6 +102,7 @@ private static GapicClass generateGenericClass(Kind kind, String name, Service s

    ClassDefinition classDef =
        ClassDefinition.builder()
+            .setFileHeader(ApacheLicenseComposer.APACHE_LICENSE)


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?

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:

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.

The way to incorporate header to the existing class is GapicClass.create(kind, classDef).addApacheLicense();

it can be independently tested in GapicClassTest without altering composer files.

Alternatives:

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.

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

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.

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

…ator-java into file-header

miraleung · 2020-08-13T00:08:29Z

src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java

@@ -62,14 +62,15 @@

  private static final String COLON = ":";
  private static final String COMMA = ",";
-  private static final String BLOCK_COMMENT_START = "/**";


Please separate the changes in #168 from this PR.

Hold this PR until #168 is merged.

src/main/java/com/google/api/generator/gapic/composer/Composer.java

miraleung · 2020-08-17T22:24:10Z

src/main/java/com/google/api/generator/engine/ast/ClassDefinition.java

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


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

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

src/main/java/com/google/api/generator/gapic/composer/Composer.java

miraleung · 2020-08-17T22:28:59Z

src/test/java/com/google/api/generator/engine/ast/ClassDefinitionTest.java

@@ -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() {


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.

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

…nto file-header

miraleung

LGTM with nits addressed.

src/main/java/com/google/api/generator/engine/ast/ClassDefinition.java

src/main/java/com/google/api/generator/gapic/utils/ApacheLicense.java

This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/4d7892ee-6265-4e89-b8a8-1c509f6b7076/targets - [ ] To automatically regenerate this PR, check this box. Source-Link: googleapis/synthtool@da29da3

xiaozhenliu-gg5 added 4 commits August 5, 2020 20:14

add file header comment

4d48254

gapic unit tests pass

712a2b9

fix bazel rules

aecb241

merge master

35cf94c

xiaozhenliu-gg5 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 6, 2020

xiaozhenliu-gg5 added 3 commits August 6, 2020 12:49

clean up

d8d9cb1

format code

4044ee4

file header class

85e87a4

xiaozhenliu-gg5 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 6, 2020

xiaozhenliu-gg5 changed the title ~~[ggj][wip]feat: support file header (apache license) for outer class definition~~ [ggj] feat: support file header (apache license) for outer class definition Aug 6, 2020

xiaozhenliu-gg5 commented Aug 6, 2020

View reviewed changes

src/main/java/com/google/api/generator/gapic/BUILD.bazel Show resolved Hide resolved

src/main/java/com/google/api/generator/gapic/composer/FileHeader.java Outdated Show resolved Hide resolved

xiaozhenliu-gg5 added 3 commits August 6, 2020 14:09

simplify create header

44c0cf5

format

23813f8

new line after file header

18f6267

xiaozhenliu-gg5 requested review from miraleung and summer-ji-eng August 6, 2020 21:20

miraleung suggested changes Aug 7, 2020

View reviewed changes

xiaozhenliu-gg5 added 7 commits August 6, 2020 23:17

merge

7b0eea2

file header is optional for class

c0d3be7

add composer test

88d71cf

Merge branch 'master' of github.com:googleapis/gapic-generator-java

45dbadb

multiple line comment

0f59ef8

Merge branch 'fix-block-comment' of github.com:googleapis/gapic-gener…

2707ea1

…ator-java into file-header

multiple line comment for header

0f34aec

xiaozhenliu-gg5 requested a review from miraleung August 9, 2020 00:01

miraleung suggested changes Aug 10, 2020

View reviewed changes

xiaozhenliu-gg5 added 3 commits August 10, 2020 16:55

rename apache license

6364167

move file header addition to gapic class

590c7c6

format

a356da9

convert change in Composer

50c1fb1

xiaozhenliu-gg5 requested a review from miraleung August 11, 2020 04:19

xiaozhenliu-gg5 added 4 commits August 11, 2020 22:49

add edge unit test

31c0468

move header addition to composer class

7ef4b4a

Merge branch 'fix-block-comment' of github.com:googleapis/gapic-gener…

ede98cb

…ator-java into file-header

format

d59e1a3

miraleung suggested changes Aug 13, 2020

View reviewed changes

xiaozhenliu-gg5 added 2 commits August 14, 2020 11:03

feedback

7a19396

merge master

7879471

xiaozhenliu-gg5 requested a review from miraleung August 16, 2020 21:50

miraleung suggested changes Aug 17, 2020

View reviewed changes

xiaozhenliu-gg5 added 2 commits August 17, 2020 16:57

Merge branch 'master' of github.com:googleapis/gapic-generator-java i…

5eccd75

…nto file-header

feedback

43f24c0

xiaozhenliu-gg5 requested a review from miraleung August 18, 2020 00:55

miraleung approved these changes Aug 19, 2020

View reviewed changes

src/main/java/com/google/api/generator/engine/ast/ClassDefinition.java Outdated Show resolved Hide resolved

src/main/java/com/google/api/generator/gapic/utils/ApacheLicense.java Outdated Show resolved Hide resolved

clean up

e051381

xiaozhenliu-gg5 merged commit 286d0dd into master Aug 19, 2020

xiaozhenliu-gg5 deleted the file-header branch September 14, 2020 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

xiaozhenliu-gg5 commented Aug 6, 2020

miraleung Aug 7, 2020

xiaozhenliu-gg5 Aug 8, 2020

xiaozhenliu-gg5 commented Aug 9, 2020 •

edited

Loading

miraleung Aug 10, 2020

xiaozhenliu-gg5 Aug 10, 2020

miraleung Aug 10, 2020

xiaozhenliu-gg5 Aug 11, 2020 •

edited

Loading

miraleung Aug 11, 2020

xiaozhenliu-gg5 Aug 12, 2020

miraleung Aug 13, 2020

xiaozhenliu-gg5 Aug 13, 2020

miraleung Aug 17, 2020

xiaozhenliu-gg5 Aug 17, 2020

miraleung Aug 17, 2020

xiaozhenliu-gg5 Aug 18, 2020 •

edited

Loading

miraleung left a comment

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

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

Conversation

xiaozhenliu-gg5 commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaozhenliu-gg5 commented Aug 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaozhenliu-gg5 Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaozhenliu-gg5 Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

miraleung left a comment

Choose a reason for hiding this comment

xiaozhenliu-gg5 commented Aug 9, 2020 •

edited

Loading

xiaozhenliu-gg5 Aug 11, 2020 •

edited

Loading

xiaozhenliu-gg5 Aug 18, 2020 •

edited

Loading