-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Rewrote Transaction#acquireConnection() methods to use async #3707
Rewrote Transaction#acquireConnection() methods to use async #3707
Conversation
@@ -50,40 +50,28 @@ module.exports = class Oracle_Transaction extends Transaction { | |||
return this.query(conn, `SAVEPOINT ${this.txid}`); | |||
} | |||
|
|||
acquireConnection(config, cb) { | |||
async acquireConnection(config, cb) { | |||
const configConnection = config && config.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.
hm
why here configConnection retrieved, but not used in transaction?
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.
This is not introduced in pull request, so shouldn't be fixed right now
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.
Oh -- it's actually referenced towards the very bottom. It helps the Transaction
decide whether or not it should release the connection.
(But, you're actually correct: even though the code is there, it doesn't actually do anything. It's either leftover from an older feature, or code that was copied unnecessarily)
The code should be functionally equivalent before and after this change. It's mostly a minor cleanup so that the next phase of refactoring will be easier.