[WIP] consolidate entity serialization #2074

vlthr · 2017-03-10T08:24:40Z

Related issue: #1950

Refactored the various implicit classes that handled serialization of entities into a single class. Entities are now serialized by unioning together the result of serializing their individual subtypes/inherited traits.

I also added a test to statically ensure that the implicit conversions work, and that the serializations are successful (but not whether the result is correct).

Review by @felixmulder

- refactored JavaEntity to be DRY with respect to repeating the serialization of fields shared by many types of entities - added tests

felixmulder

Great job, pretty much exactly what we want - as well as finding a few spots where we missed fields. Just a few nitpicks to fix and then we can merge 🎉

felixmulder · 2017-03-11T15:38:06Z

doc-tool/src/dotty/tools/dottydoc/model/JavaConverters.scala

-    case ent: Val       => ent.asJava
-    case ent: TypeAlias => ent.asJava
-    case _              => Map.empty.asJava
+  private def parseEntity(ent: Entity, extras: Map[String, _]): JMap[String, _] = {


Are we still using the extras or can we kill that parameter?

Nope, looks like none of the current callers actually pass any extras. I'll remove it!

felixmulder · 2017-03-11T15:42:32Z

doc-tool/src/dotty/tools/dottydoc/model/JavaConverters.scala

-    case _              => Map.empty.asJava
+  private def parseEntity(ent: Entity, extras: Map[String, _]): JMap[String, _] = {
+    val entity = Map(
+      "kind"     -> ent.kind,


let's kill vertical alignment now that we're writing code that may change:

+ val entity = Map( + "kind" -> ent.kind,

felixmulder · 2017-03-11T15:44:02Z

doc-tool/src/dotty/tools/dottydoc/model/JavaConverters.scala

+      )
+      case _ => Map.empty
+    }
+      (entity ++ members ++ superTypes ++ modifiers ++ typeParams ++ constructors ++ companion ++ returnValue ++ implicitlyAddedEntity ++ typeAlias ++ trt ++ df).asJava


let's make this a:

{ entity ++ members ++ ... df }.asJava

felixmulder · 2017-03-11T15:45:05Z

doc-tool/test/JavaConverterTest.scala

+import model.references.{ConstantReference, TypeReference, NoLink}
+import model.comment.Comment
+import dotty.tools.dotc.core.Symbols.NoSymbol
+import _root_.java.util.{Optional => JOptional, Map => JMap}


we can get rid of _root_ now, it was needed previously for historic (i.e. dumb) reasons.

felixmulder · 2017-03-11T15:45:44Z

doc-tool/test/JavaConverterTest.scala

+  import model.JavaConverters._
+
+  @Test def entityConversions = {
+    trait TestEntity extends Entity {


No need to create all these intermediate subclasses of entities.scala - see below

felixmulder · 2017-03-11T15:48:47Z

doc-tool/test/JavaConverterTest.scala

+    trait TestTrait extends TestModifiers with TestTypeParams with TestSuperTypes with TestMembers with TestCompanion {
+      def traitParams = List()
+    }
+    val ent = new TestEntity {}


Instead of extending and instantiating, just do:

val ent = new Entity { val symbol = ... val name = ... } ...

Is there a way to do this without repeating the fields of Entity (and some others) in each anonymous trait instantiation? I ended up making a bunch of TestX traits implementing each entity trait just to avoid repeating common fields, but I agree that it does feel a little verbose.

Maybe, but there are only a few concrete instances of the entities, eg:

Package

CaseClass

Class

Object

Trait

Def

Val

TypeAlias

See internal.scala. No need to instantiate Entity with Members et al

felixmulder · 2017-03-11T15:56:23Z

doc-tool/test/JavaConverterTest.scala

+    val implicitlyAddedEntity_serialized = implicitlyAddedEntity.asJava()
+    val typeAlias_serialized = typeAlias.asJava()
+    val def_serialized = df.asJava()
+    val trait_serialized = trt.asJava()


here we're not really testing the result of the conversion, just that the conversions apply to all entities, we could be doing:

val df = new Def { val name = "foo"; ... }.asJava assertEquals("foo", df.get("name"))

100% test coverage here might be overkill, but perhaps testing a that (pseudo code):

(Foo extends Bar).asJava.get("superTypes").asScala.toList == List(Foo)

I agree, the tests are pretty weak as they are. I'll add some checking of the actual results - especially the low-hanging fruit.

felixmulder · 2017-03-11T16:00:07Z

Feel free to include feedback as a second commit, no probs

- Added tests to ensure that the results of serialization match their pre-serialization values. - Removed unused parameter `extras` from Entity.asJava() implicit methods. - Removed _root_ imports - Fixed several code style issues

vlthr · 2017-03-14T18:45:46Z

Contents of the second commit:

Added tests to ensure that the results of serialization match their
pre-serialization values.
Removed unused parameter extras from Entity.asJava() implicit methods.
Removed _root_ imports
Fixed several code style issues

The tests are fairly comprehensive (with the most notable omission being no testing of Comment serialization), but at the same time they are fairly repetitive, which I'm worried could lead to them being out of sync with the serialization code in the future. What are your thoughts on using reflection or macros to DRY up the test code? Too much unnecessary complexity?

felixmulder · 2017-03-14T18:49:51Z

doc-tool/test/JavaConverterTest.scala

+    assertSerializedCorrectly(pkg, pkg.asJava())
+  }
+
+  def assertEach[E, A, C[E] <: Seq[E]](expected: C[E], serialized: Any, pairwiseAssertion: (E, A) => Unit) {


If we separate this into two parameter lists, do we still need the type annotation at call site?

e.g:

def map[A,B](a: A, f: A => B) = f(a) map(1, x => x * 2) // does not compile, needs type annotation def map2[A,B](a: A)(f: A => B) = f(a) map(1)(x => x * 2) // compiles

Nope, looks like we don't! Removing the type annotations significantly improves the readability of the calls.

Can I amend the commit, or would that confuse the system?

felixmulder · 2017-03-14T18:57:17Z

Well it looks like you had fun with the testing ^^

I'll have a look in the morning, for now just one feedback point already posted.

Cheers,
Felix

felixmulder · 2017-03-14T19:50:09Z

just rebase and force push, that's fine :)

…

On Tue, Mar 14, 2017 at 7:59 PM Valthor Halldorsson < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In doc-tool/test/JavaConverterTest.scala <#2074 (comment)>: > + } + assertSerializedCorrectly(vl, vl.asJava()) + val pkg = new Package { + def symbol = NoSymbol + def name = "test" + def path = "path" :: "to" :: "test" :: Nil + def comment = None + def annotations = List("test") + def parent = NonEntity + def members = trt :: typeAlias :: Nil + def superTypes = new NoLink("title", "query") :: Nil + } + assertSerializedCorrectly(pkg, pkg.asJava()) + } + + def assertEach[E, A, C[E] <: Seq[E]](expected: C[E], serialized: Any, pairwiseAssertion: (E, A) => Unit) { Nope, looks like we don't! Removing the type annotations significantly improves the readability of the calls. Can I amend the commit, or would that confuse the system? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2074 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdYwfv4F5Vh6jDxO2U3DDo4TNhKeGmRks5rluOFgaJpZM4MZGaT> .

- Added tests to ensure that the results of serialization match their pre-serialization values. - Removed unused parameter `extras` from Entity.asJava() implicit methods. - Removed _root_ imports - Fixed several code style issues

felixmulder

LGTM

felixmulder · 2017-03-15T15:35:57Z

doc-tool/test/JavaConverterTest.scala

+    assertSameSeq(expected.path, actual.get("path"))
+    assertEquals(expected.hasShortenedDocstring, actual.get("hasShortenedDocstring"))
+    // Only test if a comment is present
+    expected.comment match {


this is fine, but the idiomatic way to do this would be:

expected.comment.foreach(assertNotEquals(null, actual.get("comment")))

felixmulder · 2017-03-15T15:37:34Z

Thanks @vlthr! Happy to get this merged 🎉

consolidate entity serialization into single class

e7fe9df

- refactored JavaEntity to be DRY with respect to repeating the serialization of fields shared by many types of entities - added tests

felixmulder suggested changes Mar 11, 2017

View reviewed changes

felixmulder reviewed Mar 14, 2017

View reviewed changes

vlthr force-pushed the dottydoc-entity-implicits branch from 28f7823 to 6c92428 Compare March 14, 2017 20:46

address feedback on scala#2074

6c92428

- Added tests to ensure that the results of serialization match their pre-serialization values. - Removed unused parameter `extras` from Entity.asJava() implicit methods. - Removed _root_ imports - Fixed several code style issues

felixmulder approved these changes Mar 15, 2017

View reviewed changes

felixmulder merged commit c321653 into scala:master Mar 15, 2017

[WIP] consolidate entity serialization #2074

[WIP] consolidate entity serialization #2074

Uh oh!

Conversation

vlthr commented Mar 10, 2017

Uh oh!

felixmulder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixmulder commented Mar 11, 2017

Uh oh!

vlthr commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixmulder commented Mar 14, 2017

Uh oh!

felixmulder commented Mar 14, 2017 via email

Uh oh!

felixmulder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixmulder commented Mar 15, 2017

Uh oh!

Uh oh!

vlthr commented Mar 14, 2017 •

edited

Loading