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

Ianoc/case class tuple converters #1131

Merged
merged 7 commits into from
Dec 16, 2014
Merged

Conversation

ianoc
Copy link
Collaborator

@ianoc ianoc commented Dec 15, 2014

Tests won't pass as relies on macro code in an as yet un-released version of bijection

expandMethod(tpe.declarations
.collect { case m: MethodSymbol if m.isCaseAccessor => m },
q"""$pTree.$accessorMethod""")
case _ => List((idx: Int) => q"""tup.set(${idx}, t.$accessorMethod)""")
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't we abort here? I thought we are doing the flattening only on primitive types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thats a good question, I can see the argument going either way, you can do a setObject in cascading. Should we allow that or just throw?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the use case here? For Pig, the data people write is generally (always?) just primitive + strings right? Also, for DB it is the same thing. So, I would tend to say it would be good to error in this case.

We could take a boolean to onlySimpleTypes: Boolean or something.

import com.twitter.bijection.macros.IsCaseClass

object Macros {
def caseClassTupleSetter[T: IsCaseClass]: TupleSetter[T] = macro MacroImpl.caseClassTupleSetterImpl[T]
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should document here that these only work when the case class is "simple" (primitive + string) or nested simple.

johnynek added a commit that referenced this pull request Dec 16, 2014
@johnynek johnynek merged commit e9b4f44 into develop Dec 16, 2014
@johnynek johnynek deleted the ianoc/caseClassTupleConverters branch December 16, 2014 00:12
@coveralls
Copy link

coveralls commented Jul 1, 2016

Coverage Status

Changes Unknown when pulling af7cb52 on ianoc/caseClassTupleConverters into * on develop*.

@johnynek
Copy link
Collaborator

johnynek commented Jul 1, 2016

so mysterious... Why is coveralls pinging this 1.5 year old PR!?

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.

3 participants