-
Notifications
You must be signed in to change notification settings - Fork 40
Improve JDBC Url parsing #366
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
core/src/main/scala/scalikejdbc/async/internal/mysql/MySQLConnectionPoolImpl.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/scalikejdbc/async/internal/mysql/MySQLConnectionPoolImpl.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/scalikejdbc/async/internal/mysql/SingleAsyncMySQLConnection.scala
Outdated
Show resolved
Hide resolved
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 for promptly updating the PR! This PR already looks great to me but I'd like to wait for others' feedback for a few more days. After that, we can release a new version including this change.
@@ -26,35 +25,39 @@ private[scalikejdbc] trait JasyncConfiguration { | |||
|
|||
val defaultConfiguration = new Configuration("") |
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.
this field is no longer used, but I didnt remove it due to backwards compatibility. If that is not an issue I can remove it
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, can you add @deprecated
annotation to the public field? The since version will be 0.16.0
ok I can wait since I have an workaround. Nevertheless I used Another question: should we fix |
Also do you think the code here can now be simplified? scalikejdbc-async/core/src/main/scala/scalikejdbc/async/AsyncConnectionPoolFactory.scala Lines 48 to 75 in fc21685
|
It can still be useful for some simple use cases, so I don't think it should be deprecated immediately. But I agree that the implementation should be complete in the future. I will create a new issue on the project side. UPDATE: Here is the issue scalikejdbc/scalikejdbc#1825 |
No rush but simplifying the code is great! |
fixes #365