-
Notifications
You must be signed in to change notification settings - Fork 33
Issue#4 Adding .singleOption
#34
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
base: main
Are you sure you want to change the base?
Conversation
|
Can you run the autoformatter? |
| } | ||
|
|
||
|
|
||
| def runOption[Q, R](query: Q, fetchSize: Int = -1, queryTimeoutSeconds: Int = -1)( |
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.
Do we need this on DbApi. .single is implemented on Select and doesn't need any dedicated methods on DbApi, so I would expect singleOption would be similar
| } | ||
|
|
||
| /** | ||
| * A [[Query]] that wraps another [[Query]] but sets [[queryIsSingleRow]] to `true` |
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.
The comment should state: sets [[queryIsSingleRowOption]] to true
| * @param q | ||
| * @return | ||
| */ | ||
| def isSingleRowOption(q: Q): Boolean |
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 wonder if instead of adding another method isSingleRowOption, we should change the approach towards a method named cardinality (or something like that), that returns an enum value among: singleOptional, single or many.
I think the danger with the current approach is that we end up with two somewhat mutually exclusive cases: isSingleRow and isSingleRowOption ... what if they are both set to return true?
|
Thanks for all the comments - will work on these and put in some notes/discussions. |
|
If there's any update? |
|
Sorry - I got stuck the last time running mill command as suggested by Li, and then never got to report back. Here is some more info: Was also planning to implement the idea from @aboisvert . Would proceed once the above issue is resolved. |
|
@samikrc could you try |
Yep - bumped to 0.12.5 and that worked. Will get back with some more updates to the PR. |
Is it just the formatting that's failing for this PR? Would be nice to get this merged if it's working |
|
@NovaMage the code review feedback remains open, so it would need to be resolved before we consider merging this |
|
All: I am unable at the moment to find time to work on this. If someone wants to jump in, please let me know! |
No description provided.