-
Notifications
You must be signed in to change notification settings - Fork 37
Converted to stack-based client #37
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
Conversation
* Removed `postgres.Client.apply` * Added `Postgres.Client()` with stack-based config API * Added explicit close() on ChannelClosedException (from behind load balancer)
vkostyukov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @jeremyrsmith!
This looks great! I'm super happy to see finagle-postgres evolving. Thanks to you, it's now one step closer to the Netty 4 support!
|
|
||
| def withBinaryParams(enable: Boolean = true) = configured(BinaryParams(enable)) | ||
| def withBinaryResults(enable: Boolean = true) = configured(BinaryResults(enable)) | ||
| def database(database: String) = configured(Database(database)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
withDatabase?
| } | ||
| } | ||
|
|
||
| def dest(name: Name): Client = configured(Dest(name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need this. Dest passed as part of newService call.
| def database(database: String) = configured(Database(database)) | ||
| def withCustomTypes(customTypes: Map[Int, postgres.Client.TypeSpecifier]) = | ||
| configured(CustomTypes(Some(customTypes))) | ||
| def withDefaultTypes() = configured(CustomTypes(Some(postgres.Client.defaultTypes))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for ().
| id: String, | ||
| useSsl: Boolean, | ||
| trustManagerFactory: TrustManagerFactory = InsecureTrustManagerFactory.INSTANCE | ||
| ) extends CodecFactory[PgRequest, PgResponse] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm so happy to see this!
| // because postgres doesn't start out in TLS | ||
| ) | ||
|
|
||
| case class Client( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually provide a default instance:
val client = Client() // and then
val c = Postgres.client.with...
``| type In = PgRequest | ||
| type Out = PgResponse | ||
|
|
||
| def newRichClient(): postgres.Client = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can have newRichClient taking Name instead of grabbing it from stack params.
postgres.Client.applyPostgres.Client()with stack-based config API