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

airframe-surface, codec: #1092 Support Enum-like classes in Scala.js #1099

Merged
merged 5 commits into from
May 20, 2020

Conversation

xerial
Copy link
Member

@xerial xerial commented May 18, 2020

Support enum like classes with Surface and codec. This PR closes #1092

This support will be useful for defining RPC/Endpoint model classes with Enum-like case objects that can be shared between Scala JVM and Scala.js.

cc: @takezoe @shimamoto

@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

Merging #1099 into master will increase coverage by 0.13%.
The diff coverage is 96.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1099      +/-   ##
==========================================
+ Coverage   82.46%   82.60%   +0.13%     
==========================================
  Files         270      271       +1     
  Lines       10297    10345      +48     
  Branches      708      666      -42     
==========================================
+ Hits         8491     8545      +54     
+ Misses       1806     1800       -6     
Impacted Files Coverage Δ
...scala/wvlet/airframe/codec/JavaStandardCodec.scala 100.00% <ø> (ø)
...cala/wvlet/airframe/codec/StringUnapplyCodec.scala 60.00% <ø> (ø)
...wvlet/airframe/surface/reflect/TypeConverter.scala 55.88% <ø> (+1.47%) ⬆️
...c/main/scala/wvlet/airframe/surface/Surfaces.scala 89.00% <ø> (ø)
...rframe/surface/reflect/ReflectSurfaceFactory.scala 90.10% <94.73%> (+0.03%) ⬆️
...rc/main/scala/wvlet/airframe/codec/EnumCodec.scala 100.00% <100.00%> (ø)
...cala/wvlet/airframe/codec/MessageCodecFinder.scala 86.48% <100.00%> (+0.37%) ⬆️
...main/scala/wvlet/airframe/codec/MessageCodec.scala 85.45% <0.00%> (+0.26%) ⬆️
...rame/http/finagle/filter/HttpAccessLogFilter.scala 84.25% <0.00%> (+0.60%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93fa2eb...6fe3f6c. Read the comment docs.

@xerial xerial requested review from shimamoto and takezoe May 18, 2020 08:48
)"""
expr
case _ =>
newGenericSurfaceOf(t)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This means we cannot define Enum-like classes in the default package? But in reality, we almost always define a package.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I'll add support for unapply methods in package objects.

@shimamoto
Copy link
Collaborator

To use this support, the requirements are

  • A companion object's unapply method
  • toString should return a string that can be extracted by an unapply method

Is this correct?

@xerial
Copy link
Member Author

xerial commented May 19, 2020

@shimamoto Yes. These are correct. I'll try adding (package).unapply(s: String) too

// companion object package -> Select(Select( .... ))) tree
val ex = tl.foldLeft[Tree](Ident(TermName(p1))) { (z, n) =>
Select(z, TermName(n))
}
Copy link
Member

Choose a reason for hiding this comment

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

This generates a tree like Select(Select(Ident(TermName(java)), TermName(obj)), TermName(Object)) for java.lang.Object and it selects a companion object. I see...

Copy link
Member Author

Choose a reason for hiding this comment

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

There might be a better way to produce this expression, but I could not find it...

@xerial
Copy link
Member Author

xerial commented May 20, 2020

@shimamoto I've tried to find package object in Scala Macros, but it was difficult for me to do that. I'm not sure how frequent it is to define unapply(s:String): Option[X] in package object, though. And also, package object will be dropped in Dotty (Scala 3), so we might not need to support this use case https://dotty.epfl.ch/docs/reference/dropped-features/package-objects.html

Copy link
Collaborator

@shimamoto shimamoto left a comment

Choose a reason for hiding this comment

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

Sounds good. Thank you for taking the time to give it a try.

@xerial xerial merged commit 7ee82d7 into wvlet:master May 20, 2020
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.

airframe-surface: Surface[X].getStringExtractor: Option[String => Option[X]]
3 participants