This repository was archived by the owner on Apr 7, 2023. It is now read-only.
Initial wrapper implementation #1
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There's a weird conflict between the eslint rule and the typescript-eslint rule re: comma-dangle, so we need to make sure it's defined the same for both.
Defines our OracleWrapper class, which just simplifies the process of creating connection pools and running large queries against the database.
Since we're the primary audience for this, we'll add the @seas-computing namespace for publishing on npm.
Installs the latest version of typedoc, which requires some changes to the config file options.
rmainwork
suggested changes
Mar 9, 2021
rmainwork
left a comment
rmainwork
left a comment
There was a problem hiding this comment.
Overall, this looks great. Just a few small issues. Looking forward to having this going forward!
src/index.ts
Outdated
| public async releasePool(): Promise<void> { | ||
| if (this.connectionPool) { | ||
| try { | ||
| await this.connectionPool.close(60); |
There was a problem hiding this comment.
Might it be worth having 60 as a configurable property in the constructor (or as a class constant)? Rather than a magic number in the method like this...
Contributor
Author
There was a problem hiding this comment.
Good point. I made all the magic numbers configurable in 01f8ff1
The original implementation did not allow configuration of some of the connection options, but there is a reasonable case for making more of them configurable to accomodate different circumstances.
In addition to logging a message about the error, we can also log the full error object.
We only really need to diable the dot-notation rule in order to access private properties with bracket notation, so we should specify that in the eslint-disable-next-line directive
Since we're now merging the default options and user options, we don't really need to check the conditional assignment. That would just be testing that the `...` operator works. Additionally, we don't really need to test that the alias is being added to the poolCredentials property, and there's no point in having pointless tests.
This is more in line with our style, and generally streamlines the tests by cutting down the amount of data instantiation that needs to happen.
This is for consistency with our other projects.
Provides some additional context for why those tests would be failing, if they failed
rmainwork
suggested changes
Mar 10, 2021
rmainwork
left a comment
rmainwork
left a comment
There was a problem hiding this comment.
Looks great, still a couple more things.
The spacing for documentation was all over the place, but the most consistent inconsistency seemed to be 1.) blank line above the docblock 2.) no blank line between the docblock and the thing being documented
Codecov Report
@@ Coverage Diff @@
## main #1 +/- ##
========================================
Coverage ? 100.00%
========================================
Files ? 1
Lines ? 58
Branches ? 7
========================================
Hits ? 58
Misses ? 0
Partials ? 0 Continue to review full report at Codecov.
|
rmainwork
approved these changes
Mar 10, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Kind of breaking form by doing this initial implementation in
develop, but given that it's a very small library I don't think that's a huge issue.This sets up our
OracleWrapperclass, which is based on theOracleDBclass that's defined as part of the EAF ETL process, with some small changes aimed at making the class more generalizable. It's also in Typescript now too.The tests are all pretty basic unit tests. I had spent some time trying to get a oracle database container running for more invovled integration tests but there's just not a great solution for doing that quickly.