Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add pattern and selector to PromisePoolCluster#getConnection #3017

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

TuckerWhitehouse
Copy link
Contributor

The implementation of PromisePoolCluster#getConnection does not match the capabilities of PoolCluster#getConnection nor the definition in promise.d.ts.

This change corrects this by adding the missing pattern and selector parameters which are passed through to the underlying method.

Fixes #1381

Note: I may have missed them, but I didn't see any tests for PromisePoolCluster to update or add to with this change.

The implementation of `PromisePoolCluster#getConnection` does not match the capabilities of `PoolCluster#getConnection` nor the definition in `promise.d.ts`.

This change corrects this by adding the missing `pattern` and `selector` parameters which are passed through to the underlying method.

Fixes sidorares#1381 

Note: I may have missed them, but I didn't see any tests for PromisePoolCluster to update or add to with this change.
Copy link

codecov bot commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.13%. Comparing base (ac200fe) to head (0007f0c).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
promise.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3017   +/-   ##
=======================================
  Coverage   88.13%   88.13%           
=======================================
  Files          71       71           
  Lines       12889    12889           
  Branches     1353     1353           
=======================================
  Hits        11360    11360           
  Misses       1529     1529           
Flag Coverage Δ
compression-0 88.13% <50.00%> (ø)
compression-1 88.13% <50.00%> (ø)
tls-0 87.55% <50.00%> (ø)
tls-1 87.89% <50.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wellwelwel
Copy link
Collaborator

wellwelwel commented Sep 9, 2024

Thanks, @TuckerWhitehouse 🙋🏻‍♂️

Just confirming it:

getConnection(pattern, selector, cb) {
let namespace;
if (typeof pattern === 'function') {
cb = pattern;
namespace = this.of();
} else {
if (typeof selector === 'function') {
cb = selector;
selector = this._defaultSelector;
}
namespace = this.of(pattern, selector);
}
namespace.getConnection(cb);
}

I need to check if the typings are correct.

@wellwelwel
Copy link
Collaborator

LGTM

node-mysql2/promise.d.ts

Lines 112 to 114 in dc3a680

getConnection(): Promise<PoolConnection>;
getConnection(group: string): Promise<PoolConnection>;
getConnection(group: string, selector: string): Promise<PoolConnection>;

@wellwelwel wellwelwel merged commit ab7c49f into sidorares:master Sep 9, 2024
72 checks passed
@TuckerWhitehouse TuckerWhitehouse deleted the patch-1 branch September 9, 2024 14:04
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PromisePoolCluster missing functionality
2 participants