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

Generate definition classes for inlined objects #356

Merged
merged 1 commit into from Aug 10, 2019

Conversation

@hanny24
Copy link
Contributor

commented Jul 26, 2019

This PR bring support for generation of definition classes for objects that are inlined (they are defined with some other object definition). Current behavior is not to generate any definition class and just just io.circe.Json (scala). This PR significantly improves current situation.

Generated definitions for those classes are placed into a companion object. Name of a class is derived from a property for which the class was derived for. Please see a test if this is not clear.

A similar approach can be used for enums that are not defined on their own. Currently we use String. I plan to implement this as well, but in a separate PR to keep this one small.

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.
@hanny24

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

@hanny24 Definitely on my radar for the new week, I didn't have time to look over the weekend. From what I've seen so far, this strategy gets us closer to object deduplicaton as well, which I'm particularly interested in. Thanks for you work so far 😄

@blast-hardcheese
Copy link
Collaborator

left a comment

Some comments after an initial pass. I still haven't run it, but I'd like to say again that this is quite solid work.

}
nestedClasses.map { v =>
val finalStaticDefns = staticDefns.copy(definitions = staticDefns.definitions ++ v)
tpe.toRight("Empty entity name").map(ClassDefinition[L](clsName.last, _, defn, finalStaticDefns, parents))

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Aug 1, 2019

Collaborator

I see that fromModel calls prepareProperties which calls fromModel again. I presume this means that arbitrarily nested literals are supported. To this end, would you mind adding a runtime test (not a structure test) that ensures that the following compiles:

definitions:
  First:
    type: object
    properties:
      Second:
        type: object
        properties:
          Third:
            type: object
            properties:
              Fourth:
                type: string

I would presume that each nested object would further have other types nested inside, but ensuring that the type references are maintained would be very useful to know. I expect that test wouldn't work in Java, so we'll likely need to do some other work on our side to port what you've done over, but for the time being I believe it'll just be handled as the equivalent of a JSON literal (probably Object), so that should still compile.

This comment has been minimized.

Copy link
@hanny24

hanny24 Aug 1, 2019

Author Contributor

That's exactly right! References are maintained by the use of full names with companion objects. Maybe take a look at the new test that I added. It should make it clear.

@@ -104,6 +104,17 @@ object ScalaGenerator {
val Term.Name(name) = term
Target.pure(name)

case SelectType(typeNames) =>
val last = Type.Name(typeNames.last)

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Aug 1, 2019

Collaborator

please just

typeNames.tail.map(Term.Name.apply _)
  .foldLeft[Term.Ref](Term.Name(typeNames.head))(Term.Select.apply _)

this is super difficult to read as it is currently.

This comment has been minimized.

Copy link
@hanny24

hanny24 Aug 1, 2019

Author Contributor

But this is a different thing:) First of all, we generate Type, not a Term. This operation is used to convert (non-empty) list of identifiers to a Type:

  • if a list has single element, regular Type.Name is used
  • for any other list we must generate Type.Select, that takes Term.Ref as first argument, and Type.Name as second.

For example, given List(A,B,C) it gives you a type of A.B.C. This is used for recursively nested structures.

One alternative would be:

val result = typeNames match {
   case NonEmptyList(head, Nil) =>
       Type.Name(head)
   case NonEmptyList(head, tail) =>
       val selectors = (head :: tail.dropRight(1)).map(Term.Name(_))
       Type.Select(selectors.reduceLeft[Term.Ref](Term.Select(_,_)), Type.Name(tail.last))
}

but I'm not sure it's any better.

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Aug 1, 2019

Collaborator

Oh, you're right, I didn't notice -- I also forgot about that nuance of needing a stable identifier when constructing a Type.Select.

A slightly different approach to what you have there, maybe?

val tpe = Type.Name(typeNames.last)
val names = typeNames.init.map(Term.Name.apply _)

names match {
  case Nil => tpe
  case x :: xs =>
    val term = xs.foldLeft[Term.Ref](x)(Term.Select.apply _)
    Type.Select(term, tpe)
}

This comment has been minimized.

Copy link
@hanny24

hanny24 Aug 2, 2019

Author Contributor

Seems good to me. Changed.

@hanny24

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Are there any other concerns? If not, I can squash all the changes into single commit.

@blast-hardcheese
Copy link
Collaborator

left a comment

Thank you for your patience here, @hanny24. This looks good. I try to trend away from squash to permit git bisect to be used.

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

I'll merge whenever you say this branch is ready to be merged. If you'd like to restructure the commits in any way (atomic, etc) feel free.

@hanny24 hanny24 force-pushed the hanny24:anonymous-objects branch from 606ff73 to 0d27e3c Aug 10, 2019

@hanny24

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

It's ready to be merged now. Thanks!

@blast-hardcheese blast-hardcheese merged commit 167649e into twilio:master Aug 10, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Aug 10, 2019

Thank you! This is really exciting!

@hanny24 hanny24 deleted the hanny24:anonymous-objects branch Aug 11, 2019

@sledorze

This comment has been minimized.

Copy link

commented Aug 13, 2019

Indeed, with third party APIs that's really welcome.
Looking forward for anonymous enum support too..
btw @hanny24, is this tracked somewhere?

@hanny24

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

We do now: #370

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.