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

Abstract companion objects #158

Merged
merged 3 commits into from
Jan 11, 2019

Conversation

kelnos
Copy link
Member

@kelnos kelnos commented Jan 10, 2019

Companion objects are a feature somewhat unique to Scala, so we need to abstract them out. The per-language generators can do what they need to do from there. For example, the Java generator will turn them into static declarations on the associated class (if any).

@blast-hardcheese
Copy link
Member

@kelnos As we're still dual-shipping 2.11 and 2.12, we can't use trailing commas

Since Java doesn't have the concept of a companion object, we can't use
the `companion` member of a few of the term case classes properly.  The
closest thing Java has is static members on a regular class, so we'll
represent the companion as just a list of imports, declarations, object
members, and values, and wrap them in an object when building Scala
class files.
@kelnos
Copy link
Member Author

kelnos commented Jan 11, 2019

@blast-hardcheese ah damn, forgot. fixed, tests are passing again.

@@ -8,6 +8,12 @@ import com.twilio.guardrail.languages.LA
import com.twilio.guardrail.terms.{ ScalaTerms, SwaggerTerms }
import com.twilio.guardrail.protocol.terms.protocol.ProtocolSupportTerms

case class StaticDefns[L <: LA](className: String,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may end up being cumbersome in Java, as you'll need to check to make sure the static members are being added to the same class; at some point in the future, we may need cats.data.Ior[Defn.Class, StaticDefinitions[L]] to keep them associated. I think most of the time when we're emitting classes, there's only one in scope, so this isn't a problem at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not 100% happy with this, but at the same time I do like the fact that these things are separated. If we mash class+object generation together, then it becomes annoying for Scala. Maybe that's ok? I dunno; I'm feeling ok with this incremental change now... what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you already hit the approve button, so I guess you're ok with it too ;)

@kelnos kelnos merged commit 8ae7eca into guardrail-dev:master Jan 11, 2019
@kelnos kelnos deleted the abstract-companion-objects branch January 11, 2019 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants