-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Refactoring method addFieldIfNotExists
#4461
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
Conversation
* Using ES6 Generators syntax * Passing in the context into method `createClass`, to reuse the connection * Extending method `createClass` to reuse connections
forgot: extending method `createClass` to reuse the connection.
Codecov Report
@@ Coverage Diff @@
## master #4461 +/- ##
==========================================
+ Coverage 92.68% 92.68% +<.01%
==========================================
Files 118 118
Lines 8351 8352 +1
==========================================
+ Hits 7740 7741 +1
Misses 611 611
Continue to review full report at Codecov.
|
The code coverage thing looks like another test/coverage glitch. |
@flovilmart another one, please 😉 |
}) | ||
if (error.code === PostgresDuplicateColumnError) { | ||
// Column already exists, created by other request. Carry on to | ||
// See if it's the right type. |
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.
We should return here and not propagate the error to match original implementation. Otherwise, remove altogether.
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.
But I did match the original implementation. Was it somehow wrong?
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.
In the original implementation, the throw is in an else statement, so never executed if it enters the DuplicateColumnError check
fixing the re-throw logic.
@flovilmart I have applied the change for the re-throw logic. Weird, that review has disappeared here, must be a reviewing bug. Anyhow, good catch, and it is unfortunate that the existing tests do not cover that. |
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!
* Refactoring method `addFieldIfNotExists` * Using ES6 Generators syntax * Passing in the context into method `createClass`, to reuse the connection * Extending method `createClass` to reuse connections * Update PostgresStorageAdapter.js forgot: extending method `createClass` to reuse the connection. * Update PostgresStorageAdapter.js fixing the re-throw logic.
* Refactoring method `addFieldIfNotExists` * Using ES6 Generators syntax * Passing in the context into method `createClass`, to reuse the connection * Extending method `createClass` to reuse connections * Update PostgresStorageAdapter.js forgot: extending method `createClass` to reuse the connection. * Update PostgresStorageAdapter.js fixing the re-throw logic.
createClass
, to reuse the connectioncreateClass
to reuse connections