Update JDBCSource #898
Update JDBCSource #898
Conversation
| } | ||
| } | ||
| sealed trait JdbcDriver { | ||
| def driver: String |
There was a problem hiding this comment.
Can we make a class like: case class DriverName(className: String)
|
I went a bit crazy on the typing, more as a proof of concept. @johnynek, is this how you think things should look? I suppose it is less error prone, but it also introduces a ton of cognitive overhead for a gain I'm still not decided on. |
|
I like it. |
| protected def bigint(name : String, size : Int = 20, nullable : Boolean = false) = { | ||
| mkColumnDef(name, "BIGINT", nullable, Some(size), None) | ||
| } | ||
| protected def bigint(name: ColumnName, size : Int = 20, nullable: IsNullable = NotNullable) = |
There was a problem hiding this comment.
doesn't seem backward compatible. Will that be an issue?
There was a problem hiding this comment.
I encouraged this, but I think that ColumnName here, might be too little win. I don't see the risk of using String directly here. The more dangerous case is: def fn(name: String, dogsName: String, teamName: String) as the compiler can't help if you get an argument wrong. Maybe one thing: it is safe to use a "raw" type, if it is the only occurrence of that type (or boxed type) in the method. Thoughts on that?
There was a problem hiding this comment.
Makes sense to me. Wish scala allowed some way of enforcing named args for methods.
|
Ok, so I'd like to finish this... what do we think about:
|
|
Sounds good to me. |
| } | ||
|
|
||
| sealed abstract class ColumnType(val get: String) | ||
| private[this] case object BIGINT extends ColumnType("BIGINT") |
There was a problem hiding this comment.
why are these private? Subclasses can't use them then. Is that the intent?
|
Makes sense. |
|
So, I removed the gratuitous typing. As is it isn't backwards compatible but I am going to argue that that is ok. It'll be pretty easy to fix but because we use overriding instead of constructors to set these values, it'll be kind of ugly to make it backwards compatible in a way that is at all meaningful. Thoughts? |
| mkColumnDef(name, "BIGINT", nullable, Some(size), None) | ||
| } | ||
| protected def bigint(name: ColumnName, size: Int = 20, nullable: IsNullable = NotNullable) = | ||
| mkColumnDef(name, "DOUBLE", nullable, Some(size), None) |
There was a problem hiding this comment.
BIGINT changed to DOUBLE.
|
looks good to me other than what looks like a typo on BIGINT. |
|
@jcoveney Just to confirm - are you proposing fixing existing sources to work with this? Thanks. |
|
Crap. I think Ruban's point is correct. I think the mini-dsl here we have around declaring schemas should not change. |
|
Ok, I will change that. I am proposing that the ConnectionConfig WILL have to be typed, though. |
|
Ok made it backwards compatible |
|
merge when green. |
|
It's green :) |
|
Cool :) |
No description provided.