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

Expose connect_options for initialized pools and database on the PgConnectOptions #1897

Merged
merged 2 commits into from
Jun 16, 2022

Conversation

Nukesor
Copy link
Contributor

@Nukesor Nukesor commented Jun 7, 2022

This is a PoC PR which is related to #1853

The idea is to expose PoolOptions and ConnectionOptions after the pool has been created.

This would allow easy database deletion and modification without having to save the options used during creation for the whole duration of the program.

sqlx-core/src/postgres/options/mod.rs Outdated Show resolved Hide resolved
sqlx-core/src/pool/inner.rs Outdated Show resolved Hide resolved
@Nukesor Nukesor force-pushed the expose-pool-options branch 6 times, most recently from 32176f4 to 1599574 Compare June 10, 2022 09:14
@Nukesor
Copy link
Contributor Author

Nukesor commented Jun 10, 2022

@abonander Sorry for the quite sloppy first state of the MR, I should have properly tested it in a fully integrated scenario from the very beginning.

@svenstaro
Copy link

@Nukesor might want to update the PR name and commit title to reflect the new actual changes.

@Nukesor Nukesor changed the title Expose PoolOptions in SharedPool Expose ConnectionOptions for initialized pools and database on the PgConnectOptions Jun 13, 2022
@Nukesor Nukesor changed the title Expose ConnectionOptions for initialized pools and database on the PgConnectOptions Expose connect_options for initialized pools and database on the PgConnectOptions Jun 13, 2022
@abonander
Copy link
Collaborator

I don't think this implementation fixes #1853 as they specifically wanted PoolOptions.

@Nukesor
Copy link
Contributor Author

Nukesor commented Jun 13, 2022

True. @Thomasdezeeuww should then easily be able to implement theire desired getters in a follow-up PR.

I didn't want to expose the getter without adding specific getters, but I guess it makes sense :)

Thanks @abonander for the review 👍

@svenstaro
Copy link

svenstaro commented Jun 14, 2022

@abonander For the record, even if it doesn't fix the original issue it meant to address, it's still very helpful for a case I have right now and would therefore appreciate to see it merged. :)

@Nukesor you should remove the issue from the OP so it doesn't auto close upon merging.

@Nukesor
Copy link
Contributor Author

Nukesor commented Jun 14, 2022

I changed it from fixes to is related to

@Nukesor Nukesor requested a review from abonander June 15, 2022 20:13
@abonander abonander merged commit 21590d5 into launchbadge:master Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants