Skip to content
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

fix: PromisePoolCluster.of returns PromisePoolCluster instead of PoolNamespace #3261

Merged
merged 27 commits into from
Mar 5, 2025

Conversation

jcmartineztiempo
Copy link
Contributor

@jcmartineztiempo jcmartineztiempo commented Dec 4, 2024

Fixes #3091.

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 84.21053% with 9 lines in your changes missing coverage. Please review.

Project coverage is 88.98%. Comparing base (acba120) to head (1552305).
Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
lib/promise/pool_cluster.js 83.63% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3261      +/-   ##
==========================================
+ Coverage   88.77%   88.98%   +0.20%     
==========================================
  Files          85       86       +1     
  Lines       13448    13504      +56     
  Branches     1535     1560      +25     
==========================================
+ Hits        11939    12016      +77     
+ Misses       1509     1488      -21     
Flag Coverage Δ
compression-0 88.98% <84.21%> (+0.20%) ⬆️
compression-1 88.98% <84.21%> (+0.20%) ⬆️
static-parser-0 86.55% <84.21%> (+0.21%) ⬆️
static-parser-1 87.37% <84.21%> (+0.20%) ⬆️
tls-0 88.39% <84.21%> (+0.20%) ⬆️
tls-1 88.75% <84.21%> (+0.20%) ⬆️

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.

@jcmartineztiempo
Copy link
Contributor Author

Test added.

@wellwelwel wellwelwel changed the title Hotfix/fix 3091 fix: PromisePoolCluster.of returns PromisePoolCluster instead of PoolNamespace Dec 6, 2024
@wellwelwel
Copy link
Collaborator

Hey @jcmartineztiempo, thanks!
I couldn't pause to understand why the last test wouldn't finish, but it seems to have been fixed in recent commits.

Also, sorry for the lack of infos or support, December is usually very hectic 😅

@jcmartineztiempo
Copy link
Contributor Author

Hi @wellwelwel,

Yeah...the last test wouldn't finish due a sql syntax error in mysql 5.7. I'll look into it later.

Don't worry about support...it's my first ever pool request!!

@wellwelwel
Copy link
Collaborator

wellwelwel commented Feb 26, 2025

Hi, @jcmartineztiempo, bringing this #3091 (comment) here:

Hi, when will it be merged and released?

Because of this, I figured that the work was not yet complete:

Yeah...the last test wouldn't finish due a sql syntax error in mysql 5.7. I'll look into it later.

Sorry for the miscommunication.

Could you rebase just to trigger the recent tests into your changes?


@sidorares, just a friendly ping: could you enable the following option in the repository settings?

Screenshot 2024-11-14 at 21 12 49
  • This will display a button that allows us to automatically rebase outdated branches without conflict and ensure that the test suite always runs through the latest version.

@jcmartineztiempo
Copy link
Contributor Author

Hi,

just enabled.

Thanks

@wellwelwel
Copy link
Collaborator

Hi,

just enabled.

Thanks

Hey @jcmartineztiempo, this point is for @sidorares in MySQL2 repository 🙋🏻‍♂️

@sidorares
Copy link
Owner

@wellwelwel this is enabled now

@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 4, 2025

@jcmartineztiempo, sorry for the long delay. I've checked the behavior from mysqljs and if the typings match.

A concern was about a possible unintentional breaking change if someone checks the return using instanceof, but since it's the wrong behavior, it makes sense not to consider it that way.

A suggestion before merging (to keep consistency), would be to move the PromisePoolNamespace method to lib/promise/ and create a new file for it (pool_cluster.js) 🙋🏻‍♂️

@jcmartineztiempo
Copy link
Contributor Author

Hi @wellwelwel,

I just refactor the code to move PromisePoolNamespace to lib/promise/pool_cluster.js.

@wellwelwel wellwelwel merged commit be22202 into sidorares:master Mar 5, 2025
123 of 124 checks passed
@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 5, 2025

Thanks, @jcmartineztiempo (and welcome to the project contributors) 🚀

The fix is already available in 3.12.1-canary.be22202e version.

I've been holding the release for some time, because we depend on certain external factors. But you can follow the progress in #3377.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PoolCluster : TypeError: cb is not a function
3 participants