1
1
Fork
You've already forked shork
0

Feat/query timeout #5

Merged
ninanomenon merged 7 commits from feat/query_timeout into main 2025年01月06日 17:41:49 +01:00
ninanomenon commented 2025年01月06日 15:01:39 +01:00 (Migrated from github.com)
Copy link
No description provided.
ninanomenon (Migrated from github.com) reviewed 2025年01月06日 15:01:39 +01:00
sigmasternchen (Migrated from github.com) requested changes 2025年01月06日 15:30:45 +01:00
sigmasternchen (Migrated from github.com) left a comment
Copy link

Please take a look at my comments.

Please take a look at my comments.
sigmasternchen (Migrated from github.com) commented 2025年01月06日 15:25:10 +01:00
Copy link

I think run_query should not be public. Especially when stuff like the timeout that should be encapsulated in the Query object are used as parameters.

I think run_query should not be public. Especially when stuff like the timeout that should be encapsulated in the Query object are used as parameters.
@ -62,9 +62,15 @@ transaction(#shork_connection{pid = Pid} = Conn, Callback) ->
{error, {transaction_rolled_back, Reason}}
sigmasternchen (Migrated from github.com) commented 2025年01月06日 15:30:30 +01:00
Copy link

Maybe a name like ActualTimeout would be better. Or even better: The parameter is renamed to OptionalTimeout or something like that.

Maybe a name like `ActualTimeout` would be better. Or even better: The parameter is renamed to OptionalTimeout or something like that.
@ -373,3 +373,53 @@ pub fn transaction_commit_test() {
sigmasternchen (Migrated from github.com) commented 2025年01月06日 15:27:21 +01:00
Copy link
pub fn query_with_custom_timeout_test() {
```suggestion pub fn query_with_custom_timeout_test() { ```
sigmasternchen (Migrated from github.com) commented 2025年01月06日 15:27:36 +01:00
Copy link
pub fn query_with_too_short_custom_timeout_test() {
```suggestion pub fn query_with_too_short_custom_timeout_test() { ```
ninanomenon commented 2025年01月06日 16:56:55 +01:00 (Migrated from github.com)
Copy link

@sigmasternchen I addressed everything! Thank you for the review!

@sigmasternchen I addressed everything! Thank you for the review!
sigmasternchen (Migrated from github.com) reviewed 2025年01月06日 17:03:26 +01:00
@ -62,9 +62,15 @@ transaction(#shork_connection{pid = Pid} = Conn, Callback) ->
{error, {transaction_rolled_back, Reason}}
sigmasternchen (Migrated from github.com) commented 2025年01月06日 17:03:25 +01:00
Copy link

I'd also remove the 1.

 Timeout =
 case OptionalTimeout of
 none ->
 DefaultTimeout;
 {some, QueryTimeout} ->
 QueryTimeout
 end,
 case mysql:query(Pid, Sql, Arguments, Timeout) of
I'd also remove the 1. ```suggestion Timeout = case OptionalTimeout of none -> DefaultTimeout; {some, QueryTimeout} -> QueryTimeout end, case mysql:query(Pid, Sql, Arguments, Timeout) of ```
ninanomenon commented 2025年01月06日 17:24:28 +01:00 (Migrated from github.com)
Copy link

@sigmasternchen do you think I can merge it now?

@sigmasternchen do you think I can merge it now?
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
ninanonemon/shork!5
Reference in a new issue
ninanonemon/shork
No description provided.
Delete branch "feat/query_timeout"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?