-
Notifications
You must be signed in to change notification settings - Fork 0
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
#7: Move the code from atum-service into this repository #10
#7: Move the code from atum-service into this repository #10
Conversation
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.
- pulled
- built
- code reviewed
- unit test design - work in progress
- Integration test design - work in progress
Co-authored-by: miroslavpojer <miroslav.pojer@absa.africa>
Do we need/expect integration tests for this support library? 🤔 |
Only write it down to be part of design process. Integration test will each usage of this library for test creation. |
See related PR - adding support of JaCoCo - #12. |
|
||
package object implicits { | ||
|
||
implicit def tableOwnerConnection(in: DBConnection): Connection = in.connection |
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 understand this name. I think it's too context specific, but what this does it just an implicit conversion from one type of DB connection to another one
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.
Fix coming
* @param params - the list of parameters | ||
*/ | ||
sealed abstract class DBFunction private(functionName: String, | ||
params: ListMap[Either[Int, String], SetterFnc]) extends DBQuerySupport { |
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.
params: ListMap[Either[Int, String], SetterFnc]) extends DBQuerySupport { | |
params: ParamsMap) extends DBQuerySupport { |
you also need to import ParamsMap
so that it's visible in this scope
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.
Nice spot
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.
It would be good to document some usages / supported types etc into README, but otherwise really good work, I like the Either
approach you chose, apart from many other things :)
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.
LGTM, pulled, built, reviewed 👍
Transfer of the code from Atum repo PR
Closes #7