-
Notifications
You must be signed in to change notification settings - Fork 3
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
#301 Make SQL generators extensible so that custom generators could be plugged in without Pramen recompilation #352
Conversation
Unit Test Coverage
|
class SqlGeneratorDummy(sqlConfig: SqlConfig) extends SqlGenerator { | ||
def getSqlConfig: SqlConfig = sqlConfig | ||
|
||
override def getDtable(sql: String): String = null |
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 may consider using ???
instead of null
to avoid the usage of null
s in Scala.
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.
Interesting idea. I always use null
in dummy classes, coming from Uncle Bob mocking principles. And I use ???
when something not implemented yet, but I need the project to compile.
The difference for me is:
- When
???
is returned it means the method implementation is unfinished, and no???
should be in PRs. - When
null
is returned it means it is a dummy class and the implementation of this method is never intended.
Let me know if given this, you still prefer ???
.
|
||
def fromDriverName(driver: String, sqlConfig: SqlConfig, extraConfig: Config): SqlGenerator = { | ||
val sqlGenerator = driver match { | ||
def fromDriverName(driver: String, sqlConfig: SqlConfig): SqlGenerator = { |
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.
Did you leave the methods fromDriverName
and fromClass
with public access on purpose?
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 see, it indeed makes sense to make the private since the object has getSqlGenerator()
method that can be used in both cases.
Closes #301
The PR adds the ability to use custom JDBC drivers with custom SQL generators without recompiling Pramen. You can also use custom SQL generators with JDBC drivers that are already supported by Pramen, if there is a reason to do that.
Example JDBC reader config: