-
Notifications
You must be signed in to change notification settings - Fork 132
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
Generate definition classes for inlined objects #356
Conversation
@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 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
modules/codegen/src/main/scala/com/twilio/guardrail/ProtocolGenerator.scala
Outdated
Show resolved
Hide resolved
} | ||
nestedClasses.map { v => | ||
val finalStaticDefns = staticDefns.copy(definitions = staticDefns.definitions ++ v) | ||
tpe.toRight("Empty entity name").map(ClassDefinition[L](clsName.last, _, defn, finalStaticDefns, parents)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
modules/codegen/src/main/scala/com/twilio/guardrail/ProtocolGenerator.scala
Show resolved
Hide resolved
modules/codegen/src/main/scala/com/twilio/guardrail/ProtocolGenerator.scala
Outdated
Show resolved
Hide resolved
@@ -104,6 +104,17 @@ object ScalaGenerator { | |||
val Term.Name(name) = term | |||
Target.pure(name) | |||
|
|||
case SelectType(typeNames) => | |||
val last = Type.Name(typeNames.last) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 takesTerm.Ref
as first argument, andType.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me. Changed.
Are there any other concerns? If not, I can squash all the changes into single commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience here, @hanny24. This looks good. I try to trend away from squash to permit git bisect
to be used.
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. |
606ff73
to
0d27e3c
Compare
It's ready to be merged now. Thanks! |
Thank you! This is really exciting! |
Indeed, with third party APIs that's really welcome. |
We do now: #370 |
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