Add support for Java 8 modifier changes #244

octylFractal · 2015-03-07T03:14:39Z

Adds support for most Java 8 modifier changes.

JakeWharton · 2015-03-07T03:19:27Z

src/main/java/com/squareup/javapoet/Util.java

@@ -82,4 +84,13 @@ public static String join(String separator, List<String> parts) {
    result.addAll(b);
    return result;
  }
+
+  public static boolean hasDefaultModifier(Collection<Modifier> modifiers) {


Should not be public

Actually I guess doesn't matter, this class isn't public

swankjesse · 2015-03-07T03:46:30Z

For tests, copy a method from TypeSpecTest and modify it until it does what you need! We want to accept default on interface methods, but not elsewhere.

octylFractal · 2015-03-07T04:13:11Z

Looks like I wrote the wrong checkState, I'll add tests and attempt to fix that.

octylFractal · 2015-03-07T05:04:20Z

@swankjesse Is there any preferred order for test methods, or can I insert anywhere? Also, this can't be tested without Java 8. Is there a specific way of doing that?

swankjesse · 2015-03-07T15:29:10Z

@kenzierocks related tests are usually together, but the order isn't too important. Probably best to put these with the other tests that check for modifiers.

octylFractal · 2015-03-07T15:30:03Z

And for the Java 8 compile restriction?

swankjesse · 2015-03-07T15:30:06Z

One interesting/bad consequence of the DEFAULT constant is that testing this will cause our tests to require Java 8. @cgruber is this a problem?

JakeWharton · 2015-03-07T18:40:34Z

Hmm... what about doing an assumeTrue(isJava8()); and then Modifer.valueOf("DEFAULT")?

octylFractal · 2015-03-07T18:41:11Z

Can do.

octylFractal · 2015-03-07T19:22:47Z

Alright, that should be it.

octylFractal · 2015-03-07T19:54:53Z

Okay, found a new problem. default methods aren't abstract, so that's no longer implicit in interfaces because it would cause a checkArgument to fail. However, when there's no default, abstract is implicit, but there's no clear way to indicate this. Is there a preferred way that this should be handled?

swankjesse · 2015-03-07T20:06:55Z

@kenzierocks I think you should require either default or abstract, but not both.

octylFractal · 2015-03-07T20:07:57Z

Yes, but then it emits an abstract modifier in cases where it's implicit. I could change that checkArgument I linked above.

JakeWharton · 2015-03-07T20:12:08Z

Don't forget static. Also, while we're in here, we should support private methods on interfaces as well (Java 9).

octylFractal · 2015-03-07T20:13:29Z

Oh, that sounds cool. I think I'll rework the implicit structure a bit to ease this in the future in case even more implicitness is added/removed.

octylFractal · 2015-03-08T20:14:41Z

I changed it so that implicit modifiers are not required, which is the most minimalist change I can think of that won't break compatibility but will allow this new rule. A side effect of this is allowing the above changes to also work. I just need to add a test that checks for static on interfaces working, and also double check that private works.

swankjesse · 2015-03-08T23:59:18Z

@kenzierocks I'd like to continue requiring implicit modifiers. When Java 9 comes and interface methods can be public or private, I want the public to be in the callsite so we don't have to guess!

octylFractal · 2015-03-09T00:06:47Z

Then I'll need to think of some way to allow the implicit modifiers through, or scrap the whole system and just always print modifiers explicitly.

octylFractal · 2015-03-09T00:16:25Z

Updated the hasDefaultModifier method to cache the value of DEFAULT.

octylFractal · 2015-03-10T17:14:59Z

I'm not finding the specification for private on interface methods, would you mind showing me?

swankjesse · 2015-03-13T00:28:29Z

src/main/java/com/squareup/javapoet/Util.java

+    Modifier def = null;
+    try {
+      def = Modifier.valueOf("DEFAULT");
+    } catch (IllegalArgumentException expected) {


nit: expected –> ignored

(expected: this should happen 100% of the time. ignored: I don't care if this happens)

swankjesse · 2015-03-13T00:43:24Z

I love how this is coming together. Thanks for your patience on my strict and pedantic reviews. I think it's worth it.

octylFractal · 2015-03-13T00:47:31Z

Oh, I don't mind. The only pain I have is my formatter is off by a little bit so I'm trying not to hit format.

octylFractal · 2015-03-16T15:02:12Z

Okay, this should be good.

JakeWharton · 2015-03-17T05:03:09Z

src/main/java/com/squareup/javapoet/TypeSpec.java

+      if (kind == Kind.INTERFACE) {
+        requireExactlyOneOf(EnumSet.of(Modifier.PUBLIC, Modifier.PRIVATE),
+            fieldSpec.modifiers);
+        if (kind == Kind.INTERFACE) {


This if can be removed. You already have it above.

That was leftover from when the above if had || kind == Kind.ANNOTATION. Fixing.

octylFractal · 2015-03-17T14:28:29Z

Fixed problem, let me know if there's anything else I need to modify.

swankjesse · 2015-03-17T18:25:42Z

src/main/java/com/squareup/javapoet/TypeSpec.java

-      checkArgument(fieldSpec.modifiers.containsAll(kind.implicitFieldModifiers),
-          "%s %s.%s requires modifiers %s", kind, name, fieldSpec.name,
-          kind.implicitFieldModifiers);
+      checkState(kind != Kind.ANNOTATION, "%s %s cannot have fields", kind, name);


I love this

swankjesse · 2015-03-17T18:30:16Z

This works! I'm going to merge it as-is and send my own follow-up to you where I think we can slightly shrink the code required. (Fun java tricks!)

Add support for Java 8 modifier changes

swankjesse · 2015-03-17T18:30:26Z

Thanks for the fix, and for your patience!

swankjesse · 2015-03-17T18:49:54Z

#251

octylFractal mentioned this pull request Mar 7, 2015

Support Java 8 things #57

Closed

JakeWharton reviewed Mar 7, 2015
View reviewed changes

octylFractal force-pushed the feature/defaults branch from e66fd2a to b0f8b35 Compare March 7, 2015 19:22

octylFractal force-pushed the feature/defaults branch 2 times, most recently from 14b085d to 83dbfc5 Compare March 7, 2015 19:38

octylFractal force-pushed the feature/defaults branch 3 times, most recently from 0a89400 to 9c463b5 Compare March 8, 2015 20:10

octylFractal force-pushed the feature/defaults branch from 9c463b5 to e408f64 Compare March 9, 2015 00:15

octylFractal force-pushed the feature/defaults branch from e408f64 to 0cc0047 Compare March 9, 2015 00:17

swankjesse reviewed Mar 13, 2015
View reviewed changes

octylFractal force-pushed the feature/defaults branch 2 times, most recently from 5d3f7fe to f6ff4ab Compare March 16, 2015 14:20

octylFractal changed the title ~~Add support for default methods on interfaces~~ Add support for Java 8 modifier changes Mar 16, 2015

octylFractal force-pushed the feature/defaults branch 7 times, most recently from 53b24c7 to 894391a Compare March 16, 2015 14:59

JakeWharton reviewed Mar 17, 2015
View reviewed changes

Add support for default methods on interfaces

723a03d

octylFractal force-pushed the feature/defaults branch from 894391a to 723a03d Compare March 17, 2015 14:28

swankjesse reviewed Mar 17, 2015
View reviewed changes

swankjesse added a commit that referenced this pull request Mar 17, 2015

Merge pull request #244 from kenzierocks/feature/defaults

14216e0

Add support for Java 8 modifier changes

swankjesse merged commit 14216e0 into square:master Mar 17, 2015

octylFractal deleted the feature/defaults branch March 18, 2015 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Java 8 modifier changes #244

Add support for Java 8 modifier changes #244

octylFractal commented Mar 7, 2015

JakeWharton Mar 7, 2015

JakeWharton Mar 7, 2015

swankjesse commented Mar 7, 2015

octylFractal commented Mar 7, 2015

octylFractal commented Mar 7, 2015

swankjesse commented Mar 7, 2015

octylFractal commented Mar 7, 2015

swankjesse commented Mar 7, 2015

JakeWharton commented Mar 7, 2015

octylFractal commented Mar 7, 2015

octylFractal commented Mar 7, 2015

octylFractal commented Mar 7, 2015

swankjesse commented Mar 7, 2015

octylFractal commented Mar 7, 2015

JakeWharton commented Mar 7, 2015

octylFractal commented Mar 7, 2015

octylFractal commented Mar 8, 2015

swankjesse commented Mar 8, 2015

octylFractal commented Mar 9, 2015

octylFractal commented Mar 9, 2015

octylFractal commented Mar 10, 2015

swankjesse Mar 13, 2015

octylFractal Mar 13, 2015

swankjesse commented Mar 13, 2015

octylFractal commented Mar 13, 2015

octylFractal commented Mar 16, 2015

JakeWharton Mar 17, 2015

octylFractal Mar 17, 2015

octylFractal commented Mar 17, 2015

swankjesse Mar 17, 2015

swankjesse commented Mar 17, 2015

swankjesse commented Mar 17, 2015

swankjesse commented Mar 17, 2015

Add support for Java 8 modifier changes #244

Add support for Java 8 modifier changes #244

Conversation

octylFractal commented Mar 7, 2015

JakeWharton Mar 7, 2015

Choose a reason for hiding this comment

JakeWharton Mar 7, 2015

Choose a reason for hiding this comment

swankjesse commented Mar 7, 2015

octylFractal commented Mar 7, 2015

octylFractal commented Mar 7, 2015

swankjesse commented Mar 7, 2015

octylFractal commented Mar 7, 2015

swankjesse commented Mar 7, 2015

JakeWharton commented Mar 7, 2015

octylFractal commented Mar 7, 2015

octylFractal commented Mar 7, 2015

octylFractal commented Mar 7, 2015

swankjesse commented Mar 7, 2015

octylFractal commented Mar 7, 2015

JakeWharton commented Mar 7, 2015

octylFractal commented Mar 7, 2015

octylFractal commented Mar 8, 2015

swankjesse commented Mar 8, 2015

octylFractal commented Mar 9, 2015

octylFractal commented Mar 9, 2015

octylFractal commented Mar 10, 2015

swankjesse Mar 13, 2015

Choose a reason for hiding this comment

octylFractal Mar 13, 2015

Choose a reason for hiding this comment

swankjesse commented Mar 13, 2015

octylFractal commented Mar 13, 2015

octylFractal commented Mar 16, 2015

JakeWharton Mar 17, 2015

Choose a reason for hiding this comment

octylFractal Mar 17, 2015

Choose a reason for hiding this comment

octylFractal commented Mar 17, 2015

swankjesse Mar 17, 2015

Choose a reason for hiding this comment

swankjesse commented Mar 17, 2015

swankjesse commented Mar 17, 2015

swankjesse commented Mar 17, 2015