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

Add Additional statements for exporting individual Enum Values See #74 #81

Merged
merged 1 commit into from Jan 15, 2014

Conversation

@creativepsyco
Copy link
Contributor

@creativepsyco creativepsyco commented Jan 6, 2014

After exposing last enum method in [https://github.com/square/javapoet/pull/42] I have added additional statements which allow individual Enum values to be exposed.

Now Enums look like this:

public enum FooBarBazEnum {
    @ProtoEnum(1)
    FOO,
    @ProtoEnum(2)
    BAR,
    @ProtoEnum(3)
    BAZ;
    public static final Integer FOO_VALUE = 1;
    public static final Integer BAR_VALUE = 2;
    public static final Integer BAZ_VALUE = 3;
  }

This change would require a SNAPSHOT version of JavaWriter and I am not quite sure which tests it's going to break and need fixing.

Comments? @danrice-square

@JakeWharton
Copy link
Member

@JakeWharton JakeWharton commented Jan 6, 2014

Why wouldn't we do this instead?

public enum FooBarBazEnum {
  FOO(1),
  BAR(2),
  BAZ(3);

  private final int value;

  private FooBarBazEnum(int value) {
    this.value = value;
  }

  public int getValue() {
    return value;
  }
}

Plus this eliminates the need to avoid reflection on the annotation when we serialize to the byte representation.

@creativepsyco
Copy link
Contributor Author

@creativepsyco creativepsyco commented Jan 6, 2014

@JakeWharton Yep this sounds better, the reflection part is definitely an advantage.

Do we have a function in JavaWriter that allows us

  FOO(1),
  BAR(2),
  BAZ(3);

?

@JakeWharton
Copy link
Member

@JakeWharton JakeWharton commented Jan 7, 2014

Yes. emitEnumValue doesn't care what you give it.

writer.emitEnumValue("FOO(1)");
@creativepsyco
Copy link
Contributor Author

@creativepsyco creativepsyco commented Jan 7, 2014

@JakeWharton Changed Enum code generation. However, still would require the ability to print the last enum with a semicolon publicly via JavaWriter (introduced in square/javapoet#42) .

@@ -94,11 +94,28 @@ public void emitType(Type type, String currentType, Map<String, ?> optionsMap, b
} else if (type instanceof EnumType) {
EnumType enumType = (EnumType) type;
writer.beginType(enumType.getName(), "enum", EnumSet.of(PUBLIC));
int index = 0;
for (EnumType.Value value : enumType.getValues()) {

This comment has been minimized.

@JakeWharton

JakeWharton Jan 7, 2014
Member

Seems like we should just switch this to an indexed for loop since you're already having to do it manually.

List<Value> values = enumType.getValues();
for (int i = 0, count = values.size(); i < count; i++) {
  Value value = values.get(i);
  writer.emitEnumValue(value.getName() + "(" + value.getTag() + ")", i == count - 1);
}
@JakeWharton
Copy link
Member

@JakeWharton JakeWharton commented Jan 7, 2014

We'll get a JavaWriter release out hopefully this week.

MessageWriter.emitDocumentation(writer, value.getDocumentation());
writer.emitAnnotation(ProtoEnum.class, value.getTag());

This comment has been minimized.

@danrice-square

danrice-square Jan 9, 2014
Collaborator

We still require the @ProtoEnum annotation in order to perform deserialization. We can probably adjust serialization to use getValue() instead of using reflection on the annotation.

This comment has been minimized.

@creativepsyco

creativepsyco Jan 9, 2014
Author Contributor

I was wondering about this. Which file is responsible for the serialization and deserialization, I can make the necessary changes.

This comment has been minimized.

@danrice-square

danrice-square Jan 9, 2014
Collaborator

See EnumAdapter in the wire-runtime module. If you can make @ProtoEnum go away and still pass tests, go for it!

writer.emitStatement("this.value = value");
writer.endMethod();

writer.emitEmptyLine();

This comment has been minimized.

@danrice-square

danrice-square Jan 9, 2014
Collaborator

Move blank line below writer.emitEmptyLine();

@danrice-square
Copy link
Collaborator

@danrice-square danrice-square commented Jan 9, 2014

Note that this will require change to the reference compiler output files in order to pass tests.

@JakeWharton
Copy link
Member

@JakeWharton JakeWharton commented Jan 11, 2014

JavaWriter 2.4.0 is out. Please update the root pom.xml so we can get this PR passing.

writer.emitEmptyLine();

// Private Constructor
writer.beginMethod(null, enumType.getName(), EnumSet.of(PRIVATE), "int", "value");

This comment has been minimized.

@JakeWharton

JakeWharton Jan 11, 2014
Member

beginConstructor

// Private Constructor
writer.beginMethod(null, enumType.getName(), EnumSet.of(PRIVATE), "int", "value");
writer.emitStatement("this.value = value");
writer.endMethod();

This comment has been minimized.

@JakeWharton

JakeWharton Jan 11, 2014
Member

endConstructor

@creativepsyco
Copy link
Contributor Author

@creativepsyco creativepsyco commented Jan 11, 2014

So I open a new pull request for passing of the tests + removal of ProtoEnum ?

@JakeWharton
Copy link
Member

@JakeWharton JakeWharton commented Jan 11, 2014

Just push to the same branch and it will update the PR. We can deal with
squashing to a single commit later.
On Jan 11, 2014 1:36 PM, "Mohit Kanwal [:redDragon]" <
notifications@github.com> wrote:

So I open a new pull request for passing of the tests + removal of
ProtoEnum ?


Reply to this email directly or view it on GitHubhttps://github.com//pull/81#issuecomment-32107894
.

@creativepsyco
Copy link
Contributor Author

@creativepsyco creativepsyco commented Jan 13, 2014

I have managed to remove ProtoEnum from the reference test files and individually wire-runtime has no issues. However I have run into an indent related situation.

Currently tests are failing due to extra indentation like this one:

testRootsA(com.squareup.wire.WireCompilerTest)  Time elapsed: 0.102 sec  <<< FAILURE!
org.junit.ComparisonFailure: expected:<...j = Extension
      [.messageExtending(J.class, I.class)
      .setName("squareup.protos.roots.j")
      .setTag(1000)
]      .buildOptional...> but was:<...j = Extension
      [      .messageExtending(J.class, I.class)
            .setName("squareup.protos.roots.j")
            .setTag(1000)
      ]      .buildOptional...>
    at org.junit.Assert.assertEquals(Assert.java:125)

Any ideas? Does the new version of JavaWriter change the way continuous indents are done?

@JakeWharton
Copy link
Member

@JakeWharton JakeWharton commented Jan 13, 2014

Yes. I'll bump and fix in a separate PR this morning and then you can rebase your changes on that.

@danrice-square
Copy link
Collaborator

@danrice-square danrice-square commented Jan 13, 2014

Nicely done. Be sure to delete ProtoEnum.java and fix up any references to it in comments.

} catch (InvocationTargetException e) {
throw new RuntimeException(e);
} catch (NoSuchMethodException e) {
throw new RuntimeException(e);

This comment has been minimized.

@creativepsyco

creativepsyco Jan 13, 2014
Author Contributor

Also advise if I should throw RuntimeException if the method is not found or should I wrap

int tag = (Integer) m.invoke(value);
fromInt.put(tag, value);
toInt.put(value, tag);

in another try-catch just to be safe because enumFromInt function in Message.java essentially targets Enums in general, so we can't be sure of when to throw a RuntimeException. We can choose to not throw the RuntimeException and keep returning null in case the method getValue is not exposed in the code file.

This comment has been minimized.

@danrice-square

danrice-square Jan 13, 2014
Collaborator

I think what you have is fine. Our generated code will always have the desired field and getValue() method.

This comment has been minimized.

@JakeWharton

JakeWharton Jan 13, 2014
Member

We should switch ProtoEnum to be an interface implemented by all enum types.

public interface ProtoEnum {
  int getValue()
}

So that we can simply cast and avoid reflection.

*/
final class EnumAdapter<E extends Enum> {
private final Map<Integer, E> fromInt = new LinkedHashMap<Integer, E>();
private final Map<E, Integer> toInt = new LinkedHashMap<E, Integer>();

EnumAdapter(Class<E> type) {
// Record values for each constant annotated with '@ProtoEnum'.
// invoke getValue via Reflection to get the tag value

This comment has been minimized.

@JakeWharton

JakeWharton Jan 13, 2014
Member

Capitalize first word and add a period at the end.

@creativepsyco
Copy link
Contributor Author

@creativepsyco creativepsyco commented Jan 15, 2014

Tests are passing now.

@creativepsyco
Copy link
Contributor Author

@creativepsyco creativepsyco commented Jan 15, 2014

Squashed up my commits, I hope I did right?

@JakeWharton
Copy link
Member

@JakeWharton JakeWharton commented Jan 15, 2014

So the change looks good now, but something went wrong with the rebase. There should only be one commit and it looks like you've somehow pulled in multiple ones that aren't even yours.

@creativepsyco
Copy link
Contributor Author

@creativepsyco creativepsyco commented Jan 15, 2014

Yes I suppose that's because I pulled in changes, before doing the rebase. I will attempt to correct it, If I can't then will send a pull-request via a separate branch.

@creativepsyco
Copy link
Contributor Author

@creativepsyco creativepsyco commented Jan 15, 2014

Good to go Added overrides.

*/
final class EnumAdapter<E extends Enum> {
private final Map<Integer, E> fromInt = new LinkedHashMap<Integer, E>();
private final Map<E, Integer> toInt = new LinkedHashMap<E, Integer>();

EnumAdapter(Class<E> type) {
// Record values for each constant annotated with '@ProtoEnum'.
// Invoke getValue via Reflection to get the tag value.

This comment has been minimized.

@JakeWharton

JakeWharton Jan 15, 2014
Member

This comment is no longer true either.

* Converts values of an enum to and from integers using {@link ProtoEnum}
* annotations.
* Converts values of an enum to and from integers using Reflection on the exposed
* {@code getValue} method in code generated files.

This comment has been minimized.

@JakeWharton

JakeWharton Jan 15, 2014
Member

And it's wrong here as well

int value();
public interface ProtoEnum {
/**
* Exposed function getValue() that provides the tag value of an Enum Constant.

This comment has been minimized.

@JakeWharton

JakeWharton Jan 15, 2014
Member

Can we just say "The tag value of an enum constant." here?

@JakeWharton
Copy link
Member

@JakeWharton JakeWharton commented Jan 15, 2014

I have a few more really tiny comments. Sorry! After that I promise we'll get this in. It looks great.

Modified the way how generated Enums look like, dropped annotation.
Switch to an indexed for loop for Enum Generation.
Update pom.xml to target javawriter 2.4.0
Use beginConstructor and endConstructor functions instead of method calls + Formatting issues.
Change some test files
Remove warning about var-args being ambigous for null type during method invoke
Remove ProtoEnum annotation from AllTypes.java
Update Reference Test Output Files for removal of ProtoEnum
Clean up imports from code generation and EnumAdapter
Clean up comments and finally ProtoEnum.java from the repository.
Convert to interface ProtoEnum.
Modify test files add implements ProtoEnum clause
fix comment style.
Clean up unused imports.
Fix Checkstyle issues.
Fix up ProtoEnum test output files for import issues. [All Tests passing]
Add Override in code generated files
Fix Comments and missing overrides in test.
Fix up comments
@creativepsyco
Copy link
Contributor Author

@creativepsyco creativepsyco commented Jan 15, 2014

Fixed the comments + missed an override in one of the test files. Tests pass 😄

JakeWharton added a commit that referenced this pull request Jan 15, 2014
Add Additional statements for exporting individual Enum Values See #74
@JakeWharton JakeWharton merged commit 8c072e8 into square:master Jan 15, 2014
1 check passed
1 check passed
default The Travis CI build passed
Details
@JakeWharton
Copy link
Member

@JakeWharton JakeWharton commented Jan 15, 2014

Thanks for putting up with us. This change was worth it.

@creativepsyco
Copy link
Contributor Author

@creativepsyco creativepsyco commented Jan 15, 2014

My pleasure. Wire has been a lifesaver and easy to use. Count on me for more pull requests 👍

@daj
Copy link
Contributor

@daj daj commented Jan 16, 2014

I'd love to start using this update in my projects. Do you know roughly when the next Wire release will be coming out with this enhancement in?

JakeWharton added a commit that referenced this pull request Jul 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.