Refactor thrift type #73

benjamin-bader · 2016-10-29T06:23:57Z

This major refactoring fixes #66 and #69.

As discussed in the latter, Named goes away, and is merged in with ThriftType. ThriftType is a branchy hierarchy, and many of its elements share common structure that isn't easily expressed via direct inheritance. To that end, the new UserElement interface captures that commonality, and UserElementMixin encapsulates the logic; various types and components can implement the former by containing the latter.

Builders also get a proper refactoring with the introduction of inheritance and lots of generics garbage to make a nice API.

This is a pretty big change, though (hopefully) transparent to consumers; I'd appreciate eyeballs or, even better, testing from @hzsweers, @naturalwarren , @seanabraham, and any other folks who see this and are interested.

Also, touch up EnumMemberElement so it doesn't have an Integer value. It was always non-null by the time parsing was done, so no point to it.

ZacSweers · 2016-10-30T02:29:00Z

thrifty-schema/src/main/java/com/microsoft/thrifty/schema/NewThriftType.java

+package com.microsoft.thrifty.schema;
+
+public abstract class NewThriftType {
+    public static final BuiltinThriftType BOOL   = new BuiltinThriftType("bool");


Could these be enums instead?

ZacSweers · 2016-10-30T02:30:22Z

thrifty-schema/src/main/java/com/microsoft/thrifty/schema/UserElement.java

+    /**
+     * Gets a value indicating whether the element contains non-empty Javadoc.
+     */
+    boolean hasJavadoc();


This seems kind of necessary. How about just using nullability on documentation() to indicate presence?

The particulars are lost to the mists of time; I think it was safer/more convenient to assume that javadoc() would never return null.

ZacSweers · 2016-10-30T02:34:36Z

thrifty-schema/src/main/java/com/microsoft/thrifty/schema/UserType.java

+            ns = program.namespaces().get(NamespaceScope.ALL);
+        }
+
+        if (ns == null) {


This feels a little awkward. Most cases for us, we actually just error if there's no namespace. Just returning null would be nice in that it would fail faster or in a more obvious way anyway

ZacSweers · 2016-10-30T02:35:20Z

thrifty-schema/src/main/java/com/microsoft/thrifty/schema/BuiltinThriftType.java

@@ -12,24 +12,24 @@ public boolean isBuiltin() {

    @Override
    public <T> T accept(Visitor<T> visitor) {
-        if (this == NewThriftType.BOOL) {
+        if (this.equals(NewThriftType.BOOL)) {


Following up the enum comment elsewhere, this is a great reason why :)

ZacSweers · 2016-10-30T02:35:30Z

thrifty-schema/src/main/java/com/microsoft/thrifty/schema/NewStructType.java

+import com.microsoft.thrifty.schema.parser.StructElement;
+
+public class NewStructType extends UserType {
+


nit: double blank lines

ZacSweers · 2016-10-30T02:36:31Z

thrifty-schema/src/main/java/com/microsoft/thrifty/schema/UserType.java

+
+    @Override
+    public int hashCode() {
+        return Objects.hash(super.hashCode(), mixin, program);


ZacSweers · 2016-10-30T02:38:44Z

thrifty-schema/src/main/java/com/microsoft/thrifty/schema/Constant.java

+                element.name(),
+                element.location(),
+                element.documentation(),
+                null); // No annotations allowed on Thrift constants


Make sure to add @Nullable onto the parameter

Hm, thought that it was there?

ZacSweers · 2016-10-30T02:42:05Z

thrifty-schema/src/main/java/com/microsoft/thrifty/schema/EnumMember.java

+import com.google.common.collect.ImmutableMap;
+import com.microsoft.thrifty.schema.parser.EnumMemberElement;
+
+public class EnumMember implements UserElement {


Documentation

ZacSweers · 2016-10-30T02:43:56Z

thrifty-schema/src/main/java/com/microsoft/thrifty/schema/ThriftType.java

        return false;
+>>>>>>> Totally broken, but... Builders!  Types!  Change!


ZacSweers · 2016-10-30T02:48:54Z

thrifty-schema/src/main/java/com/microsoft/thrifty/schema/Linker.java

@@ -430,20 +436,14 @@ ThriftType resolveType(TypeElement type) {
    }

    /**
-     * Looks up a {@link Named} symbol from a given name, possibly qualified.
+     * Deprecated.
     */
    @Nullable


Add @Deprecated

ZacSweers · 2016-10-30T02:50:22Z

thrifty-schema/src/main/java/com/microsoft/thrifty/schema/TypedefType.java

@@ -55,6 +55,8 @@ void validate(Linker linker) {
        if (oldType.equals(BuiltinThriftType.VOID)) {
            linker.addError(location(), "Cannot declare a typedef of void");
        }
+
+        // Typedef cycles are validated during linking


Might want to expand on this a bit, not really clear to me currently

benjamin-bader · 2016-10-31T20:40:11Z

I'm running this branch in one of our apps now; so far, so good. I've uploaded this as 0.3.0-SNAPSHOT, so it will be easy to play with while I make sure this is baked.

I intend to leave this PR open for the week. If no problems surface, and I don't hear of any major issues from folks here, I'll land this and prepare a release next week.

yfrancis · 2016-10-31T21:45:03Z

@benjamin-bader I've set aside some time to test this tomorrow. High level changes look good.

naturalwarren · 2016-10-31T21:48:24Z

@benjamin-bader, @hzsweers and I are going to bump our Thrift code generator onto this change. Will update you with the results. This code generator creates hundreds of models and services.

yfrancis · 2016-11-03T03:18:45Z

@benjamin-bader just verified that the generator output for our thrift projects is identical to the previous release, and correct. 👍

naturalwarren · 2016-11-04T01:44:16Z

Ben I'm not able to pull these changes. Even doing a fresh clone of this repository, fetching all, and checking out refactor-thrift-type I can't find the branch.

git clone https://github.com/Microsoft/thrifty.git
git fetch --all
git checkout refactor-thrift-type
error: pathspec 'refactor-thrift-type' did not match any file(s) known to git.

benjamin-bader · 2016-11-04T05:34:21Z

@naturalwarren that's because the branch is in my own fork - replace microsoft with benjamin-bader.

benjamin-bader · 2016-11-04T20:06:05Z

In fact, @naturalwarren, here are the commands you can use:

git checkout -b benjamin-bader-refactor-thrift-type master
git pull https://github.com/benjamin-bader/thrifty.git refactor-thrift-type

benjamin-bader · 2016-11-08T01:00:02Z

OK, this has been baking for a little while and seems stable - landing now and releasing shortly.

Thanks to everyone who test-drove these changes!

benjamin-bader and others added 8 commits October 25, 2016 15:40

Begin fleshing out provision NewThriftType model.

fd843e6

Also, touch up EnumMemberElement so it doesn't have an Integer value. It was always non-null by the time parsing was done, so no point to it.

Progress, interrupted.

2ea12b5

Totally broken, but... Builders! Types! Change!

2e263d6

Clean up a few things noticed while comparing

5fd3f25

thrifty-schema tests pass, everything still broken

86e1903

Integration tests now pass

75e670c

Fix enum typecode errors in generated write methods

b1e26ae

Clean up dead code and add some comments

2401d2b

benjamin-bader added this to the 0.4.0 milestone Oct 29, 2016

msftclas added the cla-not-required label Oct 29, 2016

ZacSweers mentioned this pull request Oct 30, 2016

Add withNamespaces() API to ThriftType #74

Merged

ZacSweers reviewed Oct 30, 2016

View reviewed changes

benjamin-bader added 3 commits October 31, 2016 11:57

Rename BuiltinThriftType -> BuiltinType, add docs

257c643

Merge remote-tracking branch 'upstream/master' into refactor-thrift-type

29ab257

Add/improve Javadoc to satisfy our checkstyle

ef500dd

benjamin-bader modified the milestones: 0.3.0, 0.4.0 Oct 31, 2016

benjamin-bader merged commit fa6dea0 into microsoft:master Nov 8, 2016

benjamin-bader mentioned this pull request Nov 8, 2016

Add tests for serializing collections of enums #75

Closed

benjamin-bader deleted the refactor-thrift-type branch November 10, 2016 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor thrift type #73

Refactor thrift type #73

benjamin-bader commented Oct 29, 2016

ZacSweers Oct 30, 2016

ZacSweers Oct 30, 2016

benjamin-bader Oct 30, 2016

ZacSweers Oct 30, 2016

ZacSweers Oct 30, 2016

ZacSweers Oct 30, 2016

ZacSweers Oct 30, 2016

ZacSweers Oct 30, 2016

benjamin-bader Oct 30, 2016

ZacSweers Oct 30, 2016

ZacSweers Oct 30, 2016

ZacSweers Oct 30, 2016

ZacSweers Oct 30, 2016

benjamin-bader commented Oct 31, 2016

yfrancis commented Oct 31, 2016

naturalwarren commented Oct 31, 2016

yfrancis commented Nov 3, 2016

naturalwarren commented Nov 4, 2016 •

edited

benjamin-bader commented Nov 4, 2016

benjamin-bader commented Nov 4, 2016

benjamin-bader commented Nov 8, 2016

		import com.microsoft.thrifty.schema.parser.StructElement;

		public class NewStructType extends UserType {

		return false;
		>>>>>>> Totally broken, but... Builders! Types! Change!

Refactor thrift type #73

Refactor thrift type #73

Conversation

benjamin-bader commented Oct 29, 2016

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

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

benjamin-bader commented Oct 31, 2016

yfrancis commented Oct 31, 2016

naturalwarren commented Oct 31, 2016

yfrancis commented Nov 3, 2016

naturalwarren commented Nov 4, 2016 • edited

benjamin-bader commented Nov 4, 2016

benjamin-bader commented Nov 4, 2016

benjamin-bader commented Nov 8, 2016

naturalwarren commented Nov 4, 2016 •

edited