Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #45 +/- ##
==========================================
+ Coverage 73.75% 74.21% +0.46%
==========================================
Files 3 3
Lines 461 512 +51
==========================================
+ Hits 340 380 +40
- Misses 121 132 +11
Continue to review full report at Codecov.
|
djones6
left a comment
There was a problem hiding this comment.
I'd hate to hold up this long-awaited fix from going in, but there are some issues I'd like to understand better.
Perhaps they could be addressed in a subsequent PR, since this change is an improvement on the current behaviour.
| } | ||
| PQclear(result) | ||
| return errorMessage | ||
| setState(.idle) |
There was a problem hiding this comment.
Since setState(.idle) is the first thing done regardless of which path we take, it would be clearer if there was a single call, moved before the if status.
| } | ||
|
|
||
| PQclear(result) | ||
| setState(.idle) |
There was a problem hiding this comment.
Same comment as above - could move single setState(.idle) to above the if status
| currentResultFetcher?.hasMoreRows = false | ||
| unlockStateLock() | ||
| clearResult(nil, connection: self) | ||
| lockStateLock() |
There was a problem hiding this comment.
I think there's a potential problem here. Another thread could get in to setupForRunningQuery() in between the unlock and lock calls. The code has to be like this because the lock used is not recursive (and would otherwise result in deadlock). I wonder if we could be using an NSRecusiveLock instead?
| import Dispatch | ||
| import Foundation | ||
|
|
||
| enum ConnectionState { |
There was a problem hiding this comment.
This might be better as a nested enum inside PostgreSQLConnection, called State.
| return | ||
| } | ||
|
|
||
| if let error = setUpForRunningQuery() { |
There was a problem hiding this comment.
Naming this error implies it's an Error, which it isn't.
| unlockStateLock() | ||
| } | ||
|
|
||
| func setUpForRunningQuery() -> String? { |
There was a problem hiding this comment.
This is weird, a setup function which returns an optional String? Why not just make it throw?
|
I will defer these requested changes to the original author - @irar2 |
ianpartridge
left a comment
There was a problem hiding this comment.
We'll handle the improvements in a follow on PR.
These changes also help workaround issue 24.