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

Add target ConnectionPoolModule #412

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Conversation

fabianfett
Copy link
Collaborator

We want to land a new ConnectionPool into PostgresNIO in the comming weeks. Since this pool is abstract, let's create a target and product for it. The target and product are both underscored, to signal that we don't make any API stability guarantees.

We want to land a new ConnectionPool into PostgresNIO in the comming weeks. Since this pool is abstract, let's create a target and product for it. The target and product are both underscored, to signal that we don't make any API stability guarantees.
@fabianfett fabianfett added the semver-minor Adds new public API. label Oct 12, 2023
@fabianfett fabianfett requested a review from gwynne October 12, 2023 14:37
@Jerry-Carter
Copy link
Contributor

This change caught my attention. I'm curious, how does this relate to Issue 256? Specifically, does this mean that one or more implementations of a PostgreSQL connection pool will be included with postgres-nio in future? Likewise, will there continue to be support for developers writing their own custom connection pools as is required today?

Thanks.

@fabianfett
Copy link
Collaborator Author

fabianfett commented Oct 12, 2023

This change caught my attention. I'm curious, how does this relate to #256?

This PR means work on #256 has started.

Specifically, does this mean that one or more implementations of a PostgreSQL connection pool will be included with postgres-nio in future?

Yes we intend to add one implementation directly to PostgresNIO.

Likewise, will there continue to be support for developers writing their own custom connection pools as is required today?

We don't intend to break any adopter. However I'd be curious to learn about your use-case... Maybe the new pool is able to fulfill cater to your needs directly?

@fabianfett fabianfett added this to the ConnectionPool milestone Oct 12, 2023
@Jerry-Carter
Copy link
Contributor

Thanks and glad to hear it. We're not doing anything clever. I look forward to transitioning to the connection pool as soon as it's available. Eliminating unnecessary code is always a pleasure.

@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2023

Codecov Report

Merging #412 (8495cbe) into main (92ee156) will increase coverage by 6.54%.
Report is 2 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
+ Coverage   49.19%   55.73%   +6.54%     
==========================================
  Files         108      107       -1     
  Lines        8845     7810    -1035     
==========================================
+ Hits         4351     4353       +2     
+ Misses       4494     3457    -1037     

see 5 files with indirect coverage changes

@fabianfett fabianfett changed the title Require Swift 5.7 and add target ConnectionPoolModule Add target ConnectionPoolModule Oct 12, 2023
@fabianfett fabianfett added semver-patch No public API change. and removed semver-minor Adds new public API. labels Oct 12, 2023
@fabianfett fabianfett merged commit d4d7bed into vapor:main Oct 13, 2023
13 checks passed
@fabianfett fabianfett deleted the ff-pool-target branch October 13, 2023 07:02
@fabianfett fabianfett added the ConnectionPool Features and bugs that are related to the impl in ConnectionPoolModule label Oct 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ConnectionPool Features and bugs that are related to the impl in ConnectionPoolModule semver-patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants