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

Fix #311 BigQueryType.toTable should only take first child of field to get the type. #312

Merged
merged 1 commit into from
Oct 27, 2016

Conversation

andrewsmartin
Copy link
Contributor

No description provided.

@@ -82,7 +82,7 @@ private[types] object TypeProvider {
case List(q"case class $name(..$fields) { ..$body }") =>
val defSchema = q"override def schema: ${p(c, GModel)}.TableSchema = ${p(c, SType)}.schemaOf[$name]"
val defToPrettyString = q"override def toPrettyString(indent: Int = 0): String = ${p(c, s"$SBQ.types.SchemaUtil")}.toPrettyString(this.schema, ${name.toString}, indent)"
val fnTrait = tq"${newTypeName(s"Function${fields.size}")}[..${fields.flatMap(_.children)}, $name]"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain the difference here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, basically each param is a field that has children. For normal (non-default args), there is only one child which is the type. Default arguments have two children, where the first one is the type and the second is the default value. This just makes sure we always take the first child so we only get types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we throwing away the tail here then? Does that break default value behavior, e.g. does RecordWithDefault(10) still work without y?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nevillelyh Yeah, because this is only for generating the type of the Function trait, it's not replacing the case class definition. If you'd like I can add another test for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure that's be great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@nevillelyh
Copy link
Contributor

👍

@codecov-io
Copy link

codecov-io commented Oct 26, 2016

Current coverage is 81.87% (diff: 100%)

Merging #312 into master will increase coverage by 0.04%

@@             master       #312   diff @@
==========================================
  Files            64         64          
  Lines          2334       2334          
  Methods        2070       2079     +9   
  Messages          0          0          
  Branches        264        255     -9   
==========================================
+ Hits           1910       1911     +1   
+ Misses          424        423     -1   
  Partials          0          0          

Powered by Codecov. Last update 282b725...ee68f1c

@andrewsmartin andrewsmartin merged commit 97efa7c into master Oct 27, 2016
@andrewsmartin andrewsmartin deleted the andrew/to-table-default branch October 27, 2016 15:30
nevillelyh pushed a commit that referenced this pull request Oct 31, 2016
nevillelyh pushed a commit that referenced this pull request Jan 11, 2017
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

3 participants