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

fixing issue#99, added API and removed EventLoopGroup in README #100

Closed
wants to merge 1 commit into from

Conversation

JeffZhou215
Copy link

@JeffZhou215 JeffZhou215 commented Oct 18, 2023

Added MySQLConnection under Tests/Utilities.swift; updated the README.md.

@0xTim 0xTim requested a review from gwynne October 20, 2023 00:39
@MahdiBM
Copy link

MahdiBM commented Oct 20, 2023

Thanks for the pull request and for taking the time.

Before letting you down, I want to share that my first PR, even though it was a large effort, was rejected by @0xTim and @jdmcd because it wouldn't fit (vapor/queues#98).

Your changes are fine but they don't address the main issue, so we can't take this PR as it stands.
For example you haven't changed any of the public functions to work without accepting an EventLoop.
Adding that functions to tests doesn't help at all because that can only be used as an internal testing helper (which is unneeded) and users won't be able to take advantage of it.

If you really want to give it another try, you can take some help from this other similar PR which addressed the same problem in a similar repository: vapor/postgres-nio#389

@JeffZhou215
Copy link
Author

Thanks @MahdiBM! I will definitely spend more time on this. I truly appreciate your feedback about this issue and will take a look of the similar issue to make a new pr soon.

@MahdiBM MahdiBM closed this Oct 20, 2023
@JeffZhou215 JeffZhou215 deleted the local_branch branch October 21, 2023 02:02
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.

2 participants