Ianoc/refactor macros #186
Conversation
| override def apply(a: A) = fn(a) | ||
| override def invert(b: B) = inv(b) | ||
| } | ||
| case class MacroGeneratedInjection[A, B](fn: A => B, inv: B => Try[A]) extends Injection[A, B] with MacroGenerated { |
There was a problem hiding this comment.
while you are refactoring these, I always thought the extra level of indirection here (method calls inv.apply/fn.apply), was wasteful. We don't need it in a macro. We could just inline the method calls.
There was a problem hiding this comment.
Sure, should update to that now...
|
|
||
| object Macros { | ||
| def caseClassToTuple[T: IsCaseClass, Tup](recursivelyApply: Boolean = true): Bijection[T, Tup] = macro MacroImpl.caseClassToTupleImplWithOption[T, Tup] | ||
| def caseClassToMap[T: IsCaseClass](recursivelyApply: Boolean = true): Injection[T, Map[String, Any]] = macro MacroImpl.caseClassToMapImplWithOption[T] | ||
| def caseClassToTuple[T: IsCaseClass, Tup](recursivelyApply: Boolean = true): Bijection[T, Tup] = macro CaseClassToTuple.caseClassToTupleImplWithOption[T, Tup] |
There was a problem hiding this comment.
when I tried to call these macros, it says it could not use a default argument in the repl. Does this work for you in the REPL caseClassToMap[A]()?
There was a problem hiding this comment.
Could you verify that calling using the default argument works (in the REPL)?
There was a problem hiding this comment.
It doesn't... default args don't work in macros i believe... Will nuke.
| @@ -3,15 +3,17 @@ package com.twitter.bijection.macros | |||
| import scala.language.experimental.macros | |||
|
|
|||
| import com.twitter.bijection._ | |||
| import com.twitter.bijection.macros.common.IsCaseClass | |||
| import com.twitter.bijection.macros.impl.MacroImpl | |||
| import com.twitter.bijection.macros.impl.{ CaseClassToTuple, CaseClassToMap, IsCaseClassImpl } | |||
|
|
|||
| trait LowerPriorityMacroImplicits { | |||
There was a problem hiding this comment.
this is weird. We can't ever use these right? Should we mark them not implicit?
I think we should just not stack them. Have something like:
object MacroImplicits {
object Recursive {
}
object NonRecursive {
}
}then you import MacroImplicits.Recursive._ or something.
The current stacking is confusing.
There was a problem hiding this comment.
We can use the recursive ones in the lower priority, the maps is what I was pinging you about the other day. That doesn't make sense. I think we can hit both for the case classes since only one of them will match. (Aborting in the macro just removes it from the search so we should still be able to search for the recursive one).
There was a problem hiding this comment.
Can you add the comment about us using abort in the macro to stop the implicit search. That will be tricky if you just look at the types.
Can we remove the map one since it does not make sense?
Moves the macro's into one target, don't think we need a common separate target yet.
Attempt to remove duplicated code. Rename test classes to be more obvious what they are (too much use of A, B , both types for a func and concrete classes)